From 6edce20c3f6fc14b6f3d4a4ed0429ae2ecc9b43c Mon Sep 17 00:00:00 2001 From: Pedro Pombeiro Date: Thu, 31 Aug 2023 00:28:00 +0200 Subject: [PATCH 01/14] GraphQL: Implement failure_reason in AllJobsResolver EE: true --- app/finders/ci/jobs_finder.rb | 10 +++ app/graphql/resolvers/ci/all_jobs_resolver.rb | 11 ++- .../types/ci/job_failure_reason_enum.rb | 15 ++++ doc/api/graphql/reference/index.md | 42 +++++++++ ee/app/finders/ee/ci/jobs_finder.rb | 22 +++++ .../ee/resolvers/ci/all_jobs_resolver.rb | 35 ++++++++ ee/app/models/ee/ci/build.rb | 4 + ee/spec/finders/ee/ci/jobs_finder_spec.rb | 44 +++++++++ .../ee/resolvers/ci/all_jobs_resolver_spec.rb | 90 +++++++++++++++++++ ee/spec/models/ci/build_spec.rb | 50 +++++++++++ spec/finders/ci/jobs_finder_spec.rb | 6 +- .../resolvers/ci/all_jobs_resolver_spec.rb | 12 +-- 12 files changed, 329 insertions(+), 12 deletions(-) create mode 100644 app/graphql/types/ci/job_failure_reason_enum.rb create mode 100644 ee/app/finders/ee/ci/jobs_finder.rb create mode 100644 ee/app/graphql/ee/resolvers/ci/all_jobs_resolver.rb create mode 100644 ee/spec/finders/ee/ci/jobs_finder_spec.rb create mode 100644 ee/spec/graphql/ee/resolvers/ci/all_jobs_resolver_spec.rb diff --git a/app/finders/ci/jobs_finder.rb b/app/finders/ci/jobs_finder.rb index b35637d0e4f16b..4f9ad84f21e2f1 100644 --- a/app/finders/ci/jobs_finder.rb +++ b/app/finders/ci/jobs_finder.rb @@ -18,6 +18,7 @@ def execute builds = init_collection.order_id_desc builds = filter_by_with_artifacts(builds) builds = filter_by_runner_types(builds) + builds = filter_by_failure_reason(builds) filter_by_scope(builds) rescue Gitlab::Access::AccessDeniedError type.none @@ -84,6 +85,13 @@ def use_runner_type_filter? params[:runner_type].present? && Feature.enabled?(:admin_jobs_filter_runner_type, project, type: :ops) end + # Overriden in EE + def filter_by_failure_reason(builds) + return builds unless params[:failure_reason].present? + + raise Gitlab::Access::AccessDeniedError, 'Filtering by failure reason not available in current plan' + end + def filter_by_with_artifacts(builds) if params[:with_artifacts] builds.with_any_artifacts @@ -111,3 +119,5 @@ def jobs_by_type(relation, type) end end end + +Ci::JobsFinder.prepend_mod diff --git a/app/graphql/resolvers/ci/all_jobs_resolver.rb b/app/graphql/resolvers/ci/all_jobs_resolver.rb index 85b0f8da8772e6..86c4e343b5695b 100644 --- a/app/graphql/resolvers/ci/all_jobs_resolver.rb +++ b/app/graphql/resolvers/ci/all_jobs_resolver.rb @@ -17,15 +17,18 @@ class AllJobsResolver < BaseResolver description: 'Filter jobs by runner type if ' \ 'feature flag `:admin_jobs_filter_runner_type` is enabled.' - def resolve_with_lookahead(statuses: nil, runner_types: nil) - jobs = ::Ci::JobsFinder.new(current_user: current_user, -params: { scope: statuses, runner_type: runner_types }).execute + def resolve_with_lookahead(**args) + jobs = ::Ci::JobsFinder.new(current_user: current_user, params: params_from(args)).execute apply_lookahead(jobs) end private + def params_from(args) + { scope: args[:statuses], runner_type: args[:runner_types] } + end + def preloads { previous_stage_jobs_or_needs: [:needs, :pipeline], @@ -55,3 +58,5 @@ def nested_preloads end end end + +Resolvers::Ci::AllJobsResolver.prepend_mod diff --git a/app/graphql/types/ci/job_failure_reason_enum.rb b/app/graphql/types/ci/job_failure_reason_enum.rb new file mode 100644 index 00000000000000..3b9c13536d68db --- /dev/null +++ b/app/graphql/types/ci/job_failure_reason_enum.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +module Types + module Ci + class JobFailureReasonEnum < BaseEnum + graphql_name 'CiJobFailureReason' + + ::Enums::Ci::CommitStatus.failure_reasons.each_key do |reason| + value reason.to_s.upcase, + description: "A job that failed due to #{reason.to_s.tr('_', ' ')}.", + value: reason + end + end + end +end diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md index fe88e2e4129ca1..4932b3d7a5c223 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -462,6 +462,7 @@ four standard [pagination arguments](#connection-pagination-arguments): | Name | Type | Description | | ---- | ---- | ----------- | +| `failureReason` **{warning-solid}** | [`CiJobFailureReason`](#cijobfailurereason) | **Introduced** in 16.4. This feature is an Experiment. It can be changed or removed at any time. Filter jobs by failure reason. Currently only `RUNNER_SYSTEM_FAILURE` is supported. | | `runnerTypes` **{warning-solid}** | [`[CiRunnerType!]`](#cirunnertype) | **Introduced** in 16.4. This feature is an Experiment. It can be changed or removed at any time. Filter jobs by runner type if feature flag `:admin_jobs_filter_runner_type` is enabled. | | `statuses` | [`[CiJobStatus!]`](#cijobstatus) | Filter jobs by status. | @@ -26259,6 +26260,47 @@ Values for sorting inherited variables. | `KEY_ASC` | Key by ascending order. | | `KEY_DESC` | Key by descending order. | +### `CiJobFailureReason` + +| Value | Description | +| ----- | ----------- | +| `API_FAILURE` | A job that failed due to api failure. | +| `ARCHIVED_FAILURE` | A job that failed due to archived failure. | +| `BRIDGE_PIPELINE_IS_CHILD_PIPELINE` | A job that failed due to bridge pipeline is child pipeline. | +| `BUILDS_DISABLED` | A job that failed due to builds disabled. | +| `CI_QUOTA_EXCEEDED` | A job that failed due to ci quota exceeded. | +| `DATA_INTEGRITY_FAILURE` | A job that failed due to data integrity failure. | +| `DEPLOYMENT_REJECTED` | A job that failed due to deployment rejected. | +| `DOWNSTREAM_BRIDGE_PROJECT_NOT_FOUND` | A job that failed due to downstream bridge project not found. | +| `DOWNSTREAM_PIPELINE_CREATION_FAILED` | A job that failed due to downstream pipeline creation failed. | +| `ENVIRONMENT_CREATION_FAILURE` | A job that failed due to environment creation failure. | +| `FAILED_OUTDATED_DEPLOYMENT_JOB` | A job that failed due to failed outdated deployment job. | +| `FORWARD_DEPLOYMENT_FAILURE` | A job that failed due to forward deployment failure. | +| `INSUFFICIENT_BRIDGE_PERMISSIONS` | A job that failed due to insufficient bridge permissions. | +| `INSUFFICIENT_UPSTREAM_PERMISSIONS` | A job that failed due to insufficient upstream permissions. | +| `INVALID_BRIDGE_TRIGGER` | A job that failed due to invalid bridge trigger. | +| `IP_RESTRICTION_FAILURE` | A job that failed due to ip restriction failure. | +| `JOB_EXECUTION_TIMEOUT` | A job that failed due to job execution timeout. | +| `MISSING_DEPENDENCY_FAILURE` | A job that failed due to missing dependency failure. | +| `NO_MATCHING_RUNNER` | A job that failed due to no matching runner. | +| `PIPELINE_LOOP_DETECTED` | A job that failed due to pipeline loop detected. | +| `PROJECT_DELETED` | A job that failed due to project deleted. | +| `PROTECTED_ENVIRONMENT_FAILURE` | A job that failed due to protected environment failure. | +| `REACHED_MAX_DESCENDANT_PIPELINES_DEPTH` | A job that failed due to reached max descendant pipelines depth. | +| `REACHED_MAX_PIPELINE_HIERARCHY_SIZE` | A job that failed due to reached max pipeline hierarchy size. | +| `RUNNER_SYSTEM_FAILURE` | A job that failed due to runner system failure. | +| `RUNNER_UNSUPPORTED` | A job that failed due to runner unsupported. | +| `SCHEDULER_FAILURE` | A job that failed due to scheduler failure. | +| `SCRIPT_FAILURE` | A job that failed due to script failure. | +| `SECRETS_PROVIDER_NOT_FOUND` | A job that failed due to secrets provider not found. | +| `STALE_SCHEDULE` | A job that failed due to stale schedule. | +| `STUCK_OR_TIMEOUT_FAILURE` | A job that failed due to stuck or timeout failure. | +| `TRACE_SIZE_EXCEEDED` | A job that failed due to trace size exceeded. | +| `UNKNOWN_FAILURE` | A job that failed due to unknown failure. | +| `UNMET_PREREQUISITES` | A job that failed due to unmet prerequisites. | +| `UPSTREAM_BRIDGE_PROJECT_NOT_FOUND` | A job that failed due to upstream bridge project not found. | +| `USER_BLOCKED` | A job that failed due to user blocked. | + ### `CiJobKind` | Value | Description | diff --git a/ee/app/finders/ee/ci/jobs_finder.rb b/ee/app/finders/ee/ci/jobs_finder.rb new file mode 100644 index 00000000000000..d5a0927509b0b8 --- /dev/null +++ b/ee/app/finders/ee/ci/jobs_finder.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +module EE + module Ci + module JobsFinder + extend ::Gitlab::Utils::Override + + private + + override :filter_by_failure_reason + def filter_by_failure_reason(builds) + return builds unless use_failure_reason_filter? + + builds.recently_failed_on_instance_runner(params[:failure_reason]) # currently limited to instance runners + end + + def use_failure_reason_filter? + params[:failure_reason].present? + end + end + end +end diff --git a/ee/app/graphql/ee/resolvers/ci/all_jobs_resolver.rb b/ee/app/graphql/ee/resolvers/ci/all_jobs_resolver.rb new file mode 100644 index 00000000000000..24fed40330d31e --- /dev/null +++ b/ee/app/graphql/ee/resolvers/ci/all_jobs_resolver.rb @@ -0,0 +1,35 @@ +# frozen_string_literal: true + +module EE + module Resolvers + module Ci + module AllJobsResolver + extend ActiveSupport::Concern + extend ::Gitlab::Utils::Override + + prepended do + argument :failure_reason, ::Types::Ci::JobFailureReasonEnum, + required: false, + alpha: { milestone: '16.4' }, + description: 'Filter jobs by failure reason. Currently only `RUNNER_SYSTEM_FAILURE` is supported.' + end + + override :ready? + def ready?(failure_reason: nil, **_args) + if failure_reason.present? && failure_reason != :runner_system_failure + raise ::Gitlab::Graphql::Errors::ArgumentError, 'failureReason only supports RUNNER_SYSTEM_FAILURE' + end + + super + end + + private + + override :params_from + def params_from(args) + super.merge(failure_reason: args[:failure_reason]) + end + end + end + end +end diff --git a/ee/app/models/ee/ci/build.rb b/ee/app/models/ee/ci/build.rb index be8690467c4854..dbdc02919400b8 100644 --- a/ee/app/models/ee/ci/build.rb +++ b/ee/app/models/ee/ci/build.rb @@ -52,6 +52,10 @@ module Build .for_project_paths(project_path) end + scope :recently_failed_on_instance_runner, -> (failure_reason) do + reorder(nil).merge(::Ci::InstanceRunnerFailedJobs.recent_jobs).where(failure_reason: failure_reason) + end + state_machine :status do after_transition any => [:success, :failed, :canceled] do |build| build.run_after_commit do diff --git a/ee/spec/finders/ee/ci/jobs_finder_spec.rb b/ee/spec/finders/ee/ci/jobs_finder_spec.rb new file mode 100644 index 00000000000000..f3e7e93b2a0ae0 --- /dev/null +++ b/ee/spec/finders/ee/ci/jobs_finder_spec.rb @@ -0,0 +1,44 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Ci::JobsFinder, '#execute', feature_category: :continuous_integration do + let_it_be(:admin) { create(:user, :admin) } + + let(:params) { {} } + + context 'when project, pipeline, and runner are blank' do + subject(:finder_execute) { described_class.new(current_user: current_user, params: params).execute } + + context 'with admin and admin mode enabled', :enable_admin_mode do + let(:current_user) { admin } + + context 'with param `failure_reason` set to :runner_system_failure', :clean_gitlab_redis_shared_state, + feature_category: :runner_fleet do + let(:params) { { failure_reason: :runner_system_failure } } + + let_it_be(:instance_runner) { create(:ci_runner, :instance) } + let_it_be(:job_args) { { runner: instance_runner, failure_reason: :runner_system_failure } } + let_it_be(:job1) { create(:ci_build, :failed, finished_at: 1.minute.ago, **job_args) } + let_it_be(:job2) { create(:ci_build, :failed, finished_at: 2.minutes.ago, **job_args) } + + before do + stub_licensed_features(runner_performance_insights: true) + + ::Ci::InstanceRunnerFailedJobs.track(job1) + ::Ci::InstanceRunnerFailedJobs.track(job2) + end + + it 'returns builds tracked by InstanceRunnerFailedJobs' do + expect(::Ci::Build).to receive(:recently_failed_on_instance_runner) + .with(:runner_system_failure).once.and_call_original + + is_expected.to match([ + an_object_having_attributes(id: job1.id), + an_object_having_attributes(id: job2.id) + ]) + end + end + end + end +end diff --git a/ee/spec/graphql/ee/resolvers/ci/all_jobs_resolver_spec.rb b/ee/spec/graphql/ee/resolvers/ci/all_jobs_resolver_spec.rb new file mode 100644 index 00000000000000..beb7604aa3097f --- /dev/null +++ b/ee/spec/graphql/ee/resolvers/ci/all_jobs_resolver_spec.rb @@ -0,0 +1,90 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Resolvers::Ci::AllJobsResolver, feature_category: :continuous_integration do + include GraphqlHelpers + + let_it_be(:instance_runner) { create(:ci_runner, :instance) } + let_it_be(:successful_job) { create(:ci_build, :success, name: 'successful_job') } + let_it_be(:failed_job) { create(:ci_build, :failed, name: 'failed_job') } + let_it_be(:pending_job) { create(:ci_build, :pending, name: 'pending_job') } + let_it_be(:system_failure_job) do + create(:ci_build, :failed, failure_reason: :runner_system_failure, runner: instance_runner, + name: 'system_failure_job') + end + + let(:args) { {} } + + describe '#resolve' do + subject(:request) { resolve_jobs(args) } + + context 'with admin' do + let_it_be(:current_user) { create(:admin) } + + shared_examples 'executes as admin' do + context "with argument `failure_reason`", feature_category: :runner_fleet do + let(:args) { { failure_reason: failure_reason } } + + before_all do + project = create(:project) + project_runner = create(:ci_runner, :project, projects: [project]) + create(:ci_build, :failed, failure_reason: :runner_system_failure, runner: project_runner, + name: 'system_failure_job2') + end + + before do + stub_licensed_features(runner_performance_insights: true) + + # Mimic the transition from running to success/failed state + Ci::Build.all.each { |job| Ci::BuildFinishedWorker.new.perform(job.id) } + end + + context 'as RUNNER_SYSTEM_FAILURE' do + let(:failure_reason) { 'RUNNER_SYSTEM_FAILURE' } + + it { is_expected.to contain_exactly(system_failure_job) } + end + + context 'as RUNNER_UNSUPPORTED' do + let(:failure_reason) { 'RUNNER_UNSUPPORTED' } + + it 'generates an error' do + expect_graphql_error_to_be_created( + Gitlab::Graphql::Errors::ArgumentError, 'failureReason only supports RUNNER_SYSTEM_FAILURE' + ) do + request + end + end + end + end + end + + context 'when admin mode setting is disabled', :do_not_mock_admin_mode_setting do + it_behaves_like 'executes as admin' + end + + context 'when admin mode setting is enabled' do + context 'when in admin mode', :enable_admin_mode do + it_behaves_like 'executes as admin' + end + + context 'when not in admin mode' do + it { is_expected.to be_empty } + end + end + end + + context 'with unauthorized user' do + let(:current_user) { nil } + + it { is_expected.to be_empty } + end + end + + private + + def resolve_jobs(args = {}, context = { current_user: current_user }) + resolve(described_class, args: args, ctx: context) + end +end diff --git a/ee/spec/models/ci/build_spec.rb b/ee/spec/models/ci/build_spec.rb index 162a7875d29aa3..2f0ea5ac91eadc 100644 --- a/ee/spec/models/ci/build_spec.rb +++ b/ee/spec/models/ci/build_spec.rb @@ -670,6 +670,56 @@ end end + describe '.recently_failed_on_instance_runner', :clean_gitlab_redis_shared_state, feature_category: :runner_fleet do + subject(:recently_failed_on_instance_runner) { scope(failure_reason) } + + before do + stub_licensed_features(runner_performance_insights: true) + end + + let_it_be(:instance_runner) { create(:ci_runner, :instance) } + let_it_be(:job_args) { { runner: instance_runner, failure_reason: :runner_system_failure } } + let_it_be(:job1) { create(:ci_build, :failed, finished_at: 1.minute.ago, **job_args) } + let_it_be(:job2) { create(:ci_build, :failed, finished_at: 2.minutes.ago, **job_args) } + + def scope(failure_reason, base_scope: described_class) + base_scope.recently_failed_on_instance_runner(failure_reason) + end + + context 'with failure_reason set to :runner_system_failure' do + let(:failure_reason) { :runner_system_failure } + + it 'returns no builds' do + is_expected.to be_empty + end + + context 'with 2 jobs tracked' do + before do + ::Ci::InstanceRunnerFailedJobs.track(job2) + ::Ci::InstanceRunnerFailedJobs.track(job1) + end + + it 'returns builds tracked by InstanceRunnerFailedJobs' do + is_expected.to match([ + an_object_having_attributes(id: job1.id), + an_object_having_attributes(id: job2.id) + ]) + end + + context 'with pre-existing order on scope' do + subject(:recently_failed_on_instance_runner) { scope(:runner_system_failure, base_scope: described_class.order(:id)) } + + it 'overrides the order of returned builds' do + is_expected.to match([ + an_object_having_attributes(id: job1.id), + an_object_having_attributes(id: job2.id) + ]) + end + end + end + end + end + describe 'ci_secrets_management_available?' do subject { job.ci_secrets_management_available? } diff --git a/spec/finders/ci/jobs_finder_spec.rb b/spec/finders/ci/jobs_finder_spec.rb index e86ac65df61dd9..ef3b8c996e03e8 100644 --- a/spec/finders/ci/jobs_finder_spec.rb +++ b/spec/finders/ci/jobs_finder_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Ci::JobsFinder, '#execute', feature_category: :runner_fleet do +RSpec.describe Ci::JobsFinder, '#execute', feature_category: :continuous_integration do let_it_be(:user) { create(:user) } let_it_be(:admin) { create(:user, :admin) } let_it_be(:project) { create(:project, :private, public_builds: false) } @@ -13,8 +13,8 @@ let(:params) { {} } - context 'when project, pipeline or runner are blank' do - subject { described_class.new(current_user: current_user, params: params).execute } + context 'when project, pipeline, and runner are blank' do + subject(:finder_execute) { described_class.new(current_user: current_user, params: params).execute } context 'with admin' do let(:current_user) { admin } diff --git a/spec/graphql/resolvers/ci/all_jobs_resolver_spec.rb b/spec/graphql/resolvers/ci/all_jobs_resolver_spec.rb index 933abf31470d46..e8768258513e0b 100644 --- a/spec/graphql/resolvers/ci/all_jobs_resolver_spec.rb +++ b/spec/graphql/resolvers/ci/all_jobs_resolver_spec.rb @@ -5,6 +5,7 @@ RSpec.describe Resolvers::Ci::AllJobsResolver, feature_category: :continuous_integration do include GraphqlHelpers + let_it_be(:instance_runner) { create(:ci_runner, :instance) } let_it_be(:successful_job) { create(:ci_build, :success, name: 'successful_job') } let_it_be(:successful_job_two) { create(:ci_build, :success, name: 'successful_job_two') } let_it_be(:failed_job) { create(:ci_build, :failed, name: 'failed_job') } @@ -12,11 +13,11 @@ let(:args) { {} } - subject { resolve_jobs(args) } - describe '#resolve' do + subject(:request) { resolve_jobs(args) } + context 'with admin' do - let(:current_user) { create(:admin) } + let_it_be(:current_user) { create(:admin) } shared_examples 'executes as admin' do context "with argument `statuses`" do @@ -40,8 +41,7 @@ context "with argument `runner_types`" do let_it_be(:successful_job_with_instance_runner) do - create(:ci_build, :success, name: 'successful_job_with_instance_runner', - runner: create(:ci_runner, :instance)) + create(:ci_build, :success, name: 'successful_job_with_instance_runner', runner: instance_runner) end context 'with feature flag :admin_jobs_filter_runner_type enabled' do @@ -80,7 +80,7 @@ :ci_build, :success, name: 'successful_job_with_instance_runner', - runner: create(:ci_runner, :instance) + runner: instance_runner ) end -- GitLab From ade5c067c7fd323ea71cbb1f255b1f193aaf7da3 Mon Sep 17 00:00:00 2001 From: Pedro Pombeiro Date: Mon, 4 Sep 2023 19:02:46 +0200 Subject: [PATCH 02/14] Explicitly sidestep the ops FF that was introduced for `runnerTypes` --- app/finders/ci/jobs_finder.rb | 13 ++++- doc/api/graphql/reference/index.md | 2 +- ee/app/finders/ee/ci/jobs_finder.rb | 3 +- .../ee/resolvers/ci/all_jobs_resolver.rb | 16 ++++-- ee/app/models/ee/ci/build.rb | 4 +- ee/spec/finders/ee/ci/jobs_finder_spec.rb | 50 ++++++++++++++++--- .../ee/resolvers/ci/all_jobs_resolver_spec.rb | 48 +++++++++++++++--- 7 files changed, 115 insertions(+), 21 deletions(-) diff --git a/app/finders/ci/jobs_finder.rb b/app/finders/ci/jobs_finder.rb index 4f9ad84f21e2f1..092573e3935305 100644 --- a/app/finders/ci/jobs_finder.rb +++ b/app/finders/ci/jobs_finder.rb @@ -82,7 +82,13 @@ def filter_by_runner_types(builds) end def use_runner_type_filter? - params[:runner_type].present? && Feature.enabled?(:admin_jobs_filter_runner_type, project, type: :ops) + return false unless params[:runner_type].present? + + # we don't need to use the runner_type scope if we know that the user only cares about instance runners + # with a job failure reason + return false if use_failure_reason_filter? + + Feature.enabled?(:admin_jobs_filter_runner_type, project, type: :ops) end # Overriden in EE @@ -92,6 +98,11 @@ def filter_by_failure_reason(builds) raise Gitlab::Access::AccessDeniedError, 'Filtering by failure reason not available in current plan' end + # Overriden in EE + def use_failure_reason_filter? + false + end + def filter_by_with_artifacts(builds) if params[:with_artifacts] builds.with_any_artifacts diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md index 4932b3d7a5c223..2f889217575052 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -462,7 +462,7 @@ four standard [pagination arguments](#connection-pagination-arguments): | Name | Type | Description | | ---- | ---- | ----------- | -| `failureReason` **{warning-solid}** | [`CiJobFailureReason`](#cijobfailurereason) | **Introduced** in 16.4. This feature is an Experiment. It can be changed or removed at any time. Filter jobs by failure reason. Currently only `RUNNER_SYSTEM_FAILURE` is supported. | +| `failureReason` **{warning-solid}** | [`CiJobFailureReason`](#cijobfailurereason) | **Introduced** in 16.4. This feature is an Experiment. It can be changed or removed at any time. Filter jobs by failure reason. Currently only `RUNNER_SYSTEM_FAILURE` together with `runnerTypes: INSTANCE_TYPE` is supported. | | `runnerTypes` **{warning-solid}** | [`[CiRunnerType!]`](#cirunnertype) | **Introduced** in 16.4. This feature is an Experiment. It can be changed or removed at any time. Filter jobs by runner type if feature flag `:admin_jobs_filter_runner_type` is enabled. | | `statuses` | [`[CiJobStatus!]`](#cijobstatus) | Filter jobs by status. | diff --git a/ee/app/finders/ee/ci/jobs_finder.rb b/ee/app/finders/ee/ci/jobs_finder.rb index d5a0927509b0b8..8272acfef9c5ef 100644 --- a/ee/app/finders/ee/ci/jobs_finder.rb +++ b/ee/app/finders/ee/ci/jobs_finder.rb @@ -14,8 +14,9 @@ def filter_by_failure_reason(builds) builds.recently_failed_on_instance_runner(params[:failure_reason]) # currently limited to instance runners end + override :use_failure_reason_filter? def use_failure_reason_filter? - params[:failure_reason].present? + params[:failure_reason].present? && params[:runner_type] == ['instance_type'] end end end diff --git a/ee/app/graphql/ee/resolvers/ci/all_jobs_resolver.rb b/ee/app/graphql/ee/resolvers/ci/all_jobs_resolver.rb index 24fed40330d31e..abb58ddbf2307e 100644 --- a/ee/app/graphql/ee/resolvers/ci/all_jobs_resolver.rb +++ b/ee/app/graphql/ee/resolvers/ci/all_jobs_resolver.rb @@ -11,13 +11,21 @@ module AllJobsResolver argument :failure_reason, ::Types::Ci::JobFailureReasonEnum, required: false, alpha: { milestone: '16.4' }, - description: 'Filter jobs by failure reason. Currently only `RUNNER_SYSTEM_FAILURE` is supported.' + description: 'Filter jobs by failure reason. Currently only `RUNNER_SYSTEM_FAILURE` together with ' \ + '`runnerTypes: INSTANCE_TYPE` is supported.' end override :ready? - def ready?(failure_reason: nil, **_args) - if failure_reason.present? && failure_reason != :runner_system_failure - raise ::Gitlab::Graphql::Errors::ArgumentError, 'failureReason only supports RUNNER_SYSTEM_FAILURE' + def ready?(failure_reason: nil, runner_types: nil, **_args) + if failure_reason.present? + if failure_reason != :runner_system_failure + raise ::Gitlab::Graphql::Errors::ArgumentError, 'failureReason only supports RUNNER_SYSTEM_FAILURE' + end + + if runner_types != %w[instance_type] + raise ::Gitlab::Graphql::Errors::ArgumentError, + 'failureReason can only be used together with runnerTypes: INSTANCE_TYPE' + end end super diff --git a/ee/app/models/ee/ci/build.rb b/ee/app/models/ee/ci/build.rb index dbdc02919400b8..5bae8abfec2c42 100644 --- a/ee/app/models/ee/ci/build.rb +++ b/ee/app/models/ee/ci/build.rb @@ -53,7 +53,9 @@ module Build end scope :recently_failed_on_instance_runner, -> (failure_reason) do - reorder(nil).merge(::Ci::InstanceRunnerFailedJobs.recent_jobs).where(failure_reason: failure_reason) + reorder(nil) # Remove any default ordering as we rely on order from Ci::InstanceRunnerFailedJobs.recent_jobs + .merge(::Ci::InstanceRunnerFailedJobs.recent_jobs) + .where(failure_reason: failure_reason) end state_machine :status do diff --git a/ee/spec/finders/ee/ci/jobs_finder_spec.rb b/ee/spec/finders/ee/ci/jobs_finder_spec.rb index f3e7e93b2a0ae0..84dbef02f6bd84 100644 --- a/ee/spec/finders/ee/ci/jobs_finder_spec.rb +++ b/ee/spec/finders/ee/ci/jobs_finder_spec.rb @@ -18,9 +18,22 @@ let(:params) { { failure_reason: :runner_system_failure } } let_it_be(:instance_runner) { create(:ci_runner, :instance) } + let_it_be(:group_runner) { create(:ci_runner, :group) } + let_it_be(:project_runner) { create(:ci_runner, :project) } let_it_be(:job_args) { { runner: instance_runner, failure_reason: :runner_system_failure } } let_it_be(:job1) { create(:ci_build, :failed, finished_at: 1.minute.ago, **job_args) } let_it_be(:job2) { create(:ci_build, :failed, finished_at: 2.minutes.ago, **job_args) } + let_it_be(:job3) { create(:ci_build, :failed, finished_at: 2.minutes.ago, runner: group_runner) } + let_it_be(:job4) { create(:ci_build, :failed, finished_at: 2.minutes.ago, runner: project_runner) } + let_it_be(:job5) do + create(:ci_build, :failed, finished_at: 2.minutes.ago, runner: group_runner, + failure_reason: :runner_system_failure) + end + + let_it_be(:job6) do + create(:ci_build, :failed, finished_at: 2.minutes.ago, runner: project_runner, + failure_reason: :runner_system_failure) + end before do stub_licensed_features(runner_performance_insights: true) @@ -29,14 +42,37 @@ ::Ci::InstanceRunnerFailedJobs.track(job2) end - it 'returns builds tracked by InstanceRunnerFailedJobs' do - expect(::Ci::Build).to receive(:recently_failed_on_instance_runner) - .with(:runner_system_failure).once.and_call_original + context 'with param :runner_type set to [instance_type]' do + let(:params) { { failure_reason: :runner_system_failure, runner_type: %w[instance_type] } } + + it 'returns builds tracked by InstanceRunnerFailedJobs' do + expect(::Ci::Build).to receive(:recently_failed_on_instance_runner) + .with(:runner_system_failure).once.and_call_original + + expect(finder_execute.ids).to contain_exactly(job1.id, job2.id) + end + end + + context 'with param :runner_type set to multiple runner types', :aggregate_failures do + let(:params) { { failure_reason: :runner_system_failure, runner_type: %w[instance_type group_type] } } + + it 'ignores :failure_reason argument' do + expect(::Ci::Build).not_to receive(:recently_failed_on_instance_runner) + + expect(finder_execute.ids).to contain_exactly(job1.id, job2.id, job3.id, job5.id) + end + + context 'with feature flag :admin_jobs_filter_runner_type disabled' do + before do + stub_feature_flags(admin_jobs_filter_runner_type: false) + end + + it 'ignores :runner_type argument' do + expect(::Ci::Build).not_to receive(:recently_failed_on_instance_runner) - is_expected.to match([ - an_object_having_attributes(id: job1.id), - an_object_having_attributes(id: job2.id) - ]) + expect(finder_execute.ids).to match_array Ci::Build.all.ids + end + end end end end diff --git a/ee/spec/graphql/ee/resolvers/ci/all_jobs_resolver_spec.rb b/ee/spec/graphql/ee/resolvers/ci/all_jobs_resolver_spec.rb index beb7604aa3097f..3e78becc4511df 100644 --- a/ee/spec/graphql/ee/resolvers/ci/all_jobs_resolver_spec.rb +++ b/ee/spec/graphql/ee/resolvers/ci/all_jobs_resolver_spec.rb @@ -43,17 +43,53 @@ context 'as RUNNER_SYSTEM_FAILURE' do let(:failure_reason) { 'RUNNER_SYSTEM_FAILURE' } - it { is_expected.to contain_exactly(system_failure_job) } + it 'generates an error' do + expect_graphql_error_to_be_created(Gitlab::Graphql::Errors::ArgumentError, + 'failureReason can only be used together with runnerTypes: INSTANCE_TYPE' + ) do + request + end + end + + context 'with argument `runnerTypes`' do + let(:args) { { runner_types: runner_types, failure_reason: failure_reason } } + + context 'as INSTANCE_TYPE' do + let(:runner_types) { ['INSTANCE_TYPE'] } + + it { is_expected.to contain_exactly(system_failure_job) } + end + + context 'as GROUP_TYPE' do + let(:runner_types) { ['GROUP_TYPE'] } + + it 'generates an error' do + expect_graphql_error_to_be_created(Gitlab::Graphql::Errors::ArgumentError, + 'failureReason can only be used together with runnerTypes: INSTANCE_TYPE' + ) do + request + end + end + end + end end context 'as RUNNER_UNSUPPORTED' do let(:failure_reason) { 'RUNNER_UNSUPPORTED' } - it 'generates an error' do - expect_graphql_error_to_be_created( - Gitlab::Graphql::Errors::ArgumentError, 'failureReason only supports RUNNER_SYSTEM_FAILURE' - ) do - request + context 'with argument `runnerTypes`' do + let(:args) { { runner_types: runner_types, failure_reason: failure_reason } } + + context 'as INSTANCE_TYPE' do + let(:runner_types) { ['INSTANCE_TYPE'] } + + it 'generates an error' do + expect_graphql_error_to_be_created( + Gitlab::Graphql::Errors::ArgumentError, 'failureReason only supports RUNNER_SYSTEM_FAILURE' + ) do + request + end + end end end end -- GitLab From 67b991e84afcc1d3328e1313dd7d3c7b9d198995 Mon Sep 17 00:00:00 2001 From: Pedro Pombeiro Date: Tue, 5 Sep 2023 10:51:10 +0200 Subject: [PATCH 03/14] Check for license --- ee/app/finders/ee/ci/jobs_finder.rb | 4 ++ ee/spec/finders/ee/ci/jobs_finder_spec.rb | 52 +++++++++++++++-------- 2 files changed, 38 insertions(+), 18 deletions(-) diff --git a/ee/app/finders/ee/ci/jobs_finder.rb b/ee/app/finders/ee/ci/jobs_finder.rb index 8272acfef9c5ef..b825f39c350960 100644 --- a/ee/app/finders/ee/ci/jobs_finder.rb +++ b/ee/app/finders/ee/ci/jobs_finder.rb @@ -11,6 +11,10 @@ module JobsFinder def filter_by_failure_reason(builds) return builds unless use_failure_reason_filter? + unless License.feature_available?(:runner_performance_insights) + raise ::Gitlab::Access::AccessDeniedError, 'Filtering by failure reason not available in current plan' + end + builds.recently_failed_on_instance_runner(params[:failure_reason]) # currently limited to instance runners end diff --git a/ee/spec/finders/ee/ci/jobs_finder_spec.rb b/ee/spec/finders/ee/ci/jobs_finder_spec.rb index 84dbef02f6bd84..8ade00427a9fe4 100644 --- a/ee/spec/finders/ee/ci/jobs_finder_spec.rb +++ b/ee/spec/finders/ee/ci/jobs_finder_spec.rb @@ -36,41 +36,57 @@ end before do - stub_licensed_features(runner_performance_insights: true) + stub_licensed_features(runner_performance_insights: runner_performance_insights) ::Ci::InstanceRunnerFailedJobs.track(job1) ::Ci::InstanceRunnerFailedJobs.track(job2) end - context 'with param :runner_type set to [instance_type]' do - let(:params) { { failure_reason: :runner_system_failure, runner_type: %w[instance_type] } } + context 'without runner_performance_insights license' do + let(:runner_performance_insights) { false } - it 'returns builds tracked by InstanceRunnerFailedJobs' do - expect(::Ci::Build).to receive(:recently_failed_on_instance_runner) - .with(:runner_system_failure).once.and_call_original + context 'with param :runner_type set to [instance_type]' do + let(:params) { { failure_reason: :runner_system_failure, runner_type: %w[instance_type] } } - expect(finder_execute.ids).to contain_exactly(job1.id, job2.id) + it 'returns empty collection' do + is_expected.to be_empty + end end end - context 'with param :runner_type set to multiple runner types', :aggregate_failures do - let(:params) { { failure_reason: :runner_system_failure, runner_type: %w[instance_type group_type] } } + context 'with runner_performance_insights license' do + let(:runner_performance_insights) { true } - it 'ignores :failure_reason argument' do - expect(::Ci::Build).not_to receive(:recently_failed_on_instance_runner) + context 'with param :runner_type set to [instance_type]' do + let(:params) { { failure_reason: :runner_system_failure, runner_type: %w[instance_type] } } - expect(finder_execute.ids).to contain_exactly(job1.id, job2.id, job3.id, job5.id) - end + it 'returns builds tracked by InstanceRunnerFailedJobs' do + expect(::Ci::Build).to receive(:recently_failed_on_instance_runner) + .with(:runner_system_failure).once.and_call_original - context 'with feature flag :admin_jobs_filter_runner_type disabled' do - before do - stub_feature_flags(admin_jobs_filter_runner_type: false) + expect(finder_execute.ids).to contain_exactly(job1.id, job2.id) end + end + + context 'with param :runner_type set to multiple runner types', :aggregate_failures do + let(:params) { { failure_reason: :runner_system_failure, runner_type: %w[instance_type group_type] } } - it 'ignores :runner_type argument' do + it 'ignores :failure_reason argument' do expect(::Ci::Build).not_to receive(:recently_failed_on_instance_runner) - expect(finder_execute.ids).to match_array Ci::Build.all.ids + expect(finder_execute.ids).to contain_exactly(job1.id, job2.id, job3.id, job5.id) + end + + context 'with feature flag :admin_jobs_filter_runner_type disabled' do + before do + stub_feature_flags(admin_jobs_filter_runner_type: false) + end + + it 'ignores :runner_type argument' do + expect(::Ci::Build).not_to receive(:recently_failed_on_instance_runner) + + expect(finder_execute.ids).to match_array Ci::Build.all.ids + end end end end -- GitLab From b85513be304381abca21fb3c91372d77df5d6bf6 Mon Sep 17 00:00:00 2001 From: Pedro Pombeiro Date: Tue, 5 Sep 2023 11:21:13 +0200 Subject: [PATCH 04/14] Address MR review comments --- ee/spec/finders/ee/ci/jobs_finder_spec.rb | 113 +++++++++--------- .../ee/resolvers/ci/all_jobs_resolver_spec.rb | 8 +- .../resolvers/ci/all_jobs_resolver_spec.rb | 6 +- 3 files changed, 63 insertions(+), 64 deletions(-) diff --git a/ee/spec/finders/ee/ci/jobs_finder_spec.rb b/ee/spec/finders/ee/ci/jobs_finder_spec.rb index 8ade00427a9fe4..069e78db62aa33 100644 --- a/ee/spec/finders/ee/ci/jobs_finder_spec.rb +++ b/ee/spec/finders/ee/ci/jobs_finder_spec.rb @@ -10,82 +10,85 @@ context 'when project, pipeline, and runner are blank' do subject(:finder_execute) { described_class.new(current_user: current_user, params: params).execute } - context 'with admin and admin mode enabled', :enable_admin_mode do + context 'when current user is an admin' do let(:current_user) { admin } - context 'with param `failure_reason` set to :runner_system_failure', :clean_gitlab_redis_shared_state, - feature_category: :runner_fleet do - let(:params) { { failure_reason: :runner_system_failure } } - - let_it_be(:instance_runner) { create(:ci_runner, :instance) } - let_it_be(:group_runner) { create(:ci_runner, :group) } - let_it_be(:project_runner) { create(:ci_runner, :project) } - let_it_be(:job_args) { { runner: instance_runner, failure_reason: :runner_system_failure } } - let_it_be(:job1) { create(:ci_build, :failed, finished_at: 1.minute.ago, **job_args) } - let_it_be(:job2) { create(:ci_build, :failed, finished_at: 2.minutes.ago, **job_args) } - let_it_be(:job3) { create(:ci_build, :failed, finished_at: 2.minutes.ago, runner: group_runner) } - let_it_be(:job4) { create(:ci_build, :failed, finished_at: 2.minutes.ago, runner: project_runner) } - let_it_be(:job5) do - create(:ci_build, :failed, finished_at: 2.minutes.ago, runner: group_runner, - failure_reason: :runner_system_failure) - end + context 'when admin mode is enabled', :enable_admin_mode do + context 'with param `failure_reason` set to :runner_system_failure', :clean_gitlab_redis_shared_state, + feature_category: :runner_fleet do + let(:params) { { failure_reason: :runner_system_failure } } + + let_it_be(:instance_runner) { create(:ci_runner, :instance) } + let_it_be(:group_runner) { create(:ci_runner, :group) } + let_it_be(:project_runner) { create(:ci_runner, :project) } + let_it_be(:job_args) { { runner: instance_runner, failure_reason: :runner_system_failure } } + let_it_be(:job1) { create(:ci_build, :failed, finished_at: 1.minute.ago, **job_args) } + let_it_be(:job2) { create(:ci_build, :failed, finished_at: 2.minutes.ago, **job_args) } + let_it_be(:job7) { create(:ci_build, :failed, finished_at: 2.minutes.ago, **job_args) } + let_it_be(:job3) { create(:ci_build, :failed, finished_at: 2.minutes.ago, runner: group_runner) } + let_it_be(:job4) { create(:ci_build, :failed, finished_at: 2.minutes.ago, runner: project_runner) } + let_it_be(:job5) do + create(:ci_build, :failed, finished_at: 2.minutes.ago, runner: group_runner, + failure_reason: :runner_system_failure) + end - let_it_be(:job6) do - create(:ci_build, :failed, finished_at: 2.minutes.ago, runner: project_runner, - failure_reason: :runner_system_failure) - end + let_it_be(:job6) do + create(:ci_build, :failed, finished_at: 2.minutes.ago, runner: project_runner, + failure_reason: :runner_system_failure) + end - before do - stub_licensed_features(runner_performance_insights: runner_performance_insights) + before do + stub_licensed_features(runner_performance_insights: runner_performance_insights) - ::Ci::InstanceRunnerFailedJobs.track(job1) - ::Ci::InstanceRunnerFailedJobs.track(job2) - end + ::Ci::InstanceRunnerFailedJobs.track(job1) + ::Ci::InstanceRunnerFailedJobs.track(job2) + end - context 'without runner_performance_insights license' do - let(:runner_performance_insights) { false } + context 'without runner_performance_insights license' do + let(:runner_performance_insights) { false } - context 'with param :runner_type set to [instance_type]' do - let(:params) { { failure_reason: :runner_system_failure, runner_type: %w[instance_type] } } + context 'with param :runner_type set to [instance_type]' do + let(:params) { { failure_reason: :runner_system_failure, runner_type: %w[instance_type] } } - it 'returns empty collection' do - is_expected.to be_empty + it 'returns empty collection' do + is_expected.to be_empty + end end end - end - context 'with runner_performance_insights license' do - let(:runner_performance_insights) { true } + context 'with runner_performance_insights license' do + let(:runner_performance_insights) { true } - context 'with param :runner_type set to [instance_type]' do - let(:params) { { failure_reason: :runner_system_failure, runner_type: %w[instance_type] } } + context 'with param :runner_type set to [instance_type]' do + let(:params) { { failure_reason: :runner_system_failure, runner_type: %w[instance_type] } } - it 'returns builds tracked by InstanceRunnerFailedJobs' do - expect(::Ci::Build).to receive(:recently_failed_on_instance_runner) - .with(:runner_system_failure).once.and_call_original + it 'returns builds tracked by InstanceRunnerFailedJobs' do + expect(::Ci::Build).to receive(:recently_failed_on_instance_runner) + .with(:runner_system_failure).once.and_call_original - expect(finder_execute.ids).to contain_exactly(job1.id, job2.id) + expect(finder_execute.ids).to contain_exactly(job1.id, job2.id) + end end - end - context 'with param :runner_type set to multiple runner types', :aggregate_failures do - let(:params) { { failure_reason: :runner_system_failure, runner_type: %w[instance_type group_type] } } + context 'with param :runner_type set to multiple runner types', :aggregate_failures do + let(:params) { { failure_reason: :runner_system_failure, runner_type: %w[instance_type group_type] } } - it 'ignores :failure_reason argument' do - expect(::Ci::Build).not_to receive(:recently_failed_on_instance_runner) - - expect(finder_execute.ids).to contain_exactly(job1.id, job2.id, job3.id, job5.id) - end + it 'ignores :failure_reason argument' do + expect(::Ci::Build).not_to receive(:recently_failed_on_instance_runner) - context 'with feature flag :admin_jobs_filter_runner_type disabled' do - before do - stub_feature_flags(admin_jobs_filter_runner_type: false) + expect(finder_execute.ids).to contain_exactly(job1.id, job2.id, job3.id, job5.id) end - it 'ignores :runner_type argument' do - expect(::Ci::Build).not_to receive(:recently_failed_on_instance_runner) + context 'with feature flag :admin_jobs_filter_runner_type disabled' do + before do + stub_feature_flags(admin_jobs_filter_runner_type: false) + end + + it 'ignores :runner_type argument' do + expect(::Ci::Build).not_to receive(:recently_failed_on_instance_runner) - expect(finder_execute.ids).to match_array Ci::Build.all.ids + expect(finder_execute.ids).to match_array Ci::Build.all.ids + end end end end diff --git a/ee/spec/graphql/ee/resolvers/ci/all_jobs_resolver_spec.rb b/ee/spec/graphql/ee/resolvers/ci/all_jobs_resolver_spec.rb index 3e78becc4511df..6f7c84f429e8a7 100644 --- a/ee/spec/graphql/ee/resolvers/ci/all_jobs_resolver_spec.rb +++ b/ee/spec/graphql/ee/resolvers/ci/all_jobs_resolver_spec.rb @@ -19,7 +19,7 @@ describe '#resolve' do subject(:request) { resolve_jobs(args) } - context 'with admin' do + context 'when current user is an admin' do let_it_be(:current_user) { create(:admin) } shared_examples 'executes as admin' do @@ -110,12 +110,6 @@ end end end - - context 'with unauthorized user' do - let(:current_user) { nil } - - it { is_expected.to be_empty } - end end private diff --git a/spec/graphql/resolvers/ci/all_jobs_resolver_spec.rb b/spec/graphql/resolvers/ci/all_jobs_resolver_spec.rb index e8768258513e0b..6b9e3a484b14fa 100644 --- a/spec/graphql/resolvers/ci/all_jobs_resolver_spec.rb +++ b/spec/graphql/resolvers/ci/all_jobs_resolver_spec.rb @@ -16,7 +16,7 @@ describe '#resolve' do subject(:request) { resolve_jobs(args) } - context 'with admin' do + context 'when current user is an admin' do let_it_be(:current_user) { create(:admin) } shared_examples 'executes as admin' do @@ -132,7 +132,9 @@ end context 'with unauthorized user' do - let(:current_user) { nil } + let_it_be(:unauth_user) { create(:user) } + + let(:current_user) { unauth_user } it { is_expected.to be_empty } end -- GitLab From f46286934b762df8d1344a8aa3dcc17cd990f4a9 Mon Sep 17 00:00:00 2001 From: Pedro Pombeiro Date: Tue, 5 Sep 2023 12:22:01 +0200 Subject: [PATCH 05/14] Try to fix undercoverage job --- app/finders/ci/jobs_finder.rb | 9 ------- ee/app/finders/ee/ci/jobs_finder.rb | 16 +++++++----- ee/spec/finders/ee/ci/jobs_finder_spec.rb | 12 ++++++--- spec/finders/ci/jobs_finder_spec.rb | 32 ++++++++++++----------- 4 files changed, 35 insertions(+), 34 deletions(-) diff --git a/app/finders/ci/jobs_finder.rb b/app/finders/ci/jobs_finder.rb index 092573e3935305..c88e1500212fc5 100644 --- a/app/finders/ci/jobs_finder.rb +++ b/app/finders/ci/jobs_finder.rb @@ -84,10 +84,6 @@ def filter_by_runner_types(builds) def use_runner_type_filter? return false unless params[:runner_type].present? - # we don't need to use the runner_type scope if we know that the user only cares about instance runners - # with a job failure reason - return false if use_failure_reason_filter? - Feature.enabled?(:admin_jobs_filter_runner_type, project, type: :ops) end @@ -98,11 +94,6 @@ def filter_by_failure_reason(builds) raise Gitlab::Access::AccessDeniedError, 'Filtering by failure reason not available in current plan' end - # Overriden in EE - def use_failure_reason_filter? - false - end - def filter_by_with_artifacts(builds) if params[:with_artifacts] builds.with_any_artifacts diff --git a/ee/app/finders/ee/ci/jobs_finder.rb b/ee/app/finders/ee/ci/jobs_finder.rb index b825f39c350960..5eabd9dcd074f3 100644 --- a/ee/app/finders/ee/ci/jobs_finder.rb +++ b/ee/app/finders/ee/ci/jobs_finder.rb @@ -9,16 +9,20 @@ module JobsFinder override :filter_by_failure_reason def filter_by_failure_reason(builds) - return builds unless use_failure_reason_filter? - - unless License.feature_available?(:runner_performance_insights) - raise ::Gitlab::Access::AccessDeniedError, 'Filtering by failure reason not available in current plan' - end + return super unless use_failure_reason_filter? && License.feature_available?(:runner_performance_insights) builds.recently_failed_on_instance_runner(params[:failure_reason]) # currently limited to instance runners end - override :use_failure_reason_filter? + override :use_runner_type_filter? + def use_runner_type_filter? + # we don't need to use the runner_type scope if we know that the user only cares about instance runners + # with a job failure reason + return false if use_failure_reason_filter? + + super + end + def use_failure_reason_filter? params[:failure_reason].present? && params[:runner_type] == ['instance_type'] end diff --git a/ee/spec/finders/ee/ci/jobs_finder_spec.rb b/ee/spec/finders/ee/ci/jobs_finder_spec.rb index 069e78db62aa33..eee151c7f33022 100644 --- a/ee/spec/finders/ee/ci/jobs_finder_spec.rb +++ b/ee/spec/finders/ee/ci/jobs_finder_spec.rb @@ -47,6 +47,10 @@ context 'without runner_performance_insights license' do let(:runner_performance_insights) { false } + it 'returns empty collection' do + is_expected.to be_empty + end + context 'with param :runner_type set to [instance_type]' do let(:params) { { failure_reason: :runner_system_failure, runner_type: %w[instance_type] } } @@ -73,10 +77,10 @@ context 'with param :runner_type set to multiple runner types', :aggregate_failures do let(:params) { { failure_reason: :runner_system_failure, runner_type: %w[instance_type group_type] } } - it 'ignores :failure_reason argument' do + it 'returns empty collection' do expect(::Ci::Build).not_to receive(:recently_failed_on_instance_runner) - expect(finder_execute.ids).to contain_exactly(job1.id, job2.id, job3.id, job5.id) + expect(finder_execute.ids).to be_empty end context 'with feature flag :admin_jobs_filter_runner_type disabled' do @@ -84,10 +88,10 @@ stub_feature_flags(admin_jobs_filter_runner_type: false) end - it 'ignores :runner_type argument' do + it 'returns empty collection' do expect(::Ci::Build).not_to receive(:recently_failed_on_instance_runner) - expect(finder_execute.ids).to match_array Ci::Build.all.ids + expect(finder_execute.ids).to be_empty end end end diff --git a/spec/finders/ci/jobs_finder_spec.rb b/spec/finders/ci/jobs_finder_spec.rb index ef3b8c996e03e8..57046baafab7d0 100644 --- a/spec/finders/ci/jobs_finder_spec.rb +++ b/spec/finders/ci/jobs_finder_spec.rb @@ -278,28 +278,30 @@ let_it_be(:runner) { create(:ci_runner, :project, projects: [project]) } let_it_be(:job_4) { create(:ci_build, :success, runner: runner) } - subject { described_class.new(current_user: user, runner: runner, params: params).execute } + subject(:execute) { described_class.new(current_user: user, runner: runner, params: params).execute } - context 'with admin and admin mode enabled', :enable_admin_mode do + context 'when current user is an admin' do let(:user) { admin } - it 'returns jobs for the specified project' do - expect(subject).to match_array([job_4]) - end + context 'when admin mode is enabled', :enable_admin_mode do + it 'returns jobs for the specified project' do + expect(subject).to contain_exactly job_4 + end - context "with params" do - using RSpec::Parameterized::TableSyntax + context 'with params' do + using RSpec::Parameterized::TableSyntax - where(:param_runner_type, :param_scope, :expected_jobs) do - 'project_type' | 'success' | lazy { [job_4] } - 'instance_type' | nil | lazy { [] } - nil | 'pending' | lazy { [] } - end + where(:param_runner_type, :param_scope, :expected_jobs) do + 'project_type' | 'success' | lazy { [job_4] } + 'instance_type' | nil | lazy { [] } + nil | 'pending' | lazy { [] } + end - with_them do - let(:params) { { runner_type: param_runner_type, scope: param_scope } } + with_them do + let(:params) { { runner_type: param_runner_type, scope: param_scope } } - it { is_expected.to match_array(expected_jobs) } + it { is_expected.to match_array(expected_jobs) } + end end end end -- GitLab From 9f5bf465c75cddb214fd8efa3a2fb906306def6e Mon Sep 17 00:00:00 2001 From: Pedro Pombeiro Date: Tue, 5 Sep 2023 19:36:55 +0200 Subject: [PATCH 06/14] Address MR review comments by vshushlin --- app/finders/ci/jobs_finder.rb | 32 ++--- app/graphql/resolvers/ci/all_jobs_resolver.rb | 4 +- ee/app/finders/ee/ci/jobs_finder.rb | 16 ++- .../ee/resolvers/ci/all_jobs_resolver.rb | 4 +- ee/spec/finders/ee/ci/jobs_finder_spec.rb | 20 +-- .../ee/resolvers/ci/all_jobs_resolver_spec.rb | 120 ------------------ ee/spec/requests/api/graphql/ci/jobs_spec.rb | 115 +++++++++++++++++ 7 files changed, 155 insertions(+), 156 deletions(-) delete mode 100644 ee/spec/graphql/ee/resolvers/ci/all_jobs_resolver_spec.rb create mode 100644 ee/spec/requests/api/graphql/ci/jobs_spec.rb diff --git a/app/finders/ci/jobs_finder.rb b/app/finders/ci/jobs_finder.rb index c88e1500212fc5..efacd8143bc88a 100644 --- a/app/finders/ci/jobs_finder.rb +++ b/app/finders/ci/jobs_finder.rb @@ -16,10 +16,7 @@ def initialize(current_user:, pipeline: nil, project: nil, runner: nil, params: def execute builds = init_collection.order_id_desc - builds = filter_by_with_artifacts(builds) - builds = filter_by_runner_types(builds) - builds = filter_by_failure_reason(builds) - filter_by_scope(builds) + filter_builds(builds) rescue Gitlab::Access::AccessDeniedError type.none end @@ -60,6 +57,13 @@ def pipeline_jobs params[:include_retried] ? jobs_scope : jobs_scope.latest end + # Overriden in EE + def filter_builds(builds) + builds = filter_by_with_artifacts(builds) + builds = filter_by_runner_types(builds) + filter_by_scope(builds) + end + def filter_by_scope(builds) return filter_by_statuses!(builds) if params[:scope].is_a?(Array) @@ -81,25 +85,15 @@ def filter_by_runner_types(builds) builds.with_runner_type(params[:runner_type]) end - def use_runner_type_filter? - return false unless params[:runner_type].present? - - Feature.enabled?(:admin_jobs_filter_runner_type, project, type: :ops) - end - # Overriden in EE - def filter_by_failure_reason(builds) - return builds unless params[:failure_reason].present? - - raise Gitlab::Access::AccessDeniedError, 'Filtering by failure reason not available in current plan' + def use_runner_type_filter? + params[:runner_type].present? && Feature.enabled?(:admin_jobs_filter_runner_type, project, type: :ops) end def filter_by_with_artifacts(builds) - if params[:with_artifacts] - builds.with_any_artifacts - else - builds - end + return builds.with_any_artifacts if params[:with_artifacts] + + builds end def filter_by_statuses!(builds) diff --git a/app/graphql/resolvers/ci/all_jobs_resolver.rb b/app/graphql/resolvers/ci/all_jobs_resolver.rb index 86c4e343b5695b..3012a7defa63f9 100644 --- a/app/graphql/resolvers/ci/all_jobs_resolver.rb +++ b/app/graphql/resolvers/ci/all_jobs_resolver.rb @@ -18,14 +18,14 @@ class AllJobsResolver < BaseResolver 'feature flag `:admin_jobs_filter_runner_type` is enabled.' def resolve_with_lookahead(**args) - jobs = ::Ci::JobsFinder.new(current_user: current_user, params: params_from(args)).execute + jobs = ::Ci::JobsFinder.new(current_user: current_user, params: params_data(args)).execute apply_lookahead(jobs) end private - def params_from(args) + def params_data(args) { scope: args[:statuses], runner_type: args[:runner_types] } end diff --git a/ee/app/finders/ee/ci/jobs_finder.rb b/ee/app/finders/ee/ci/jobs_finder.rb index 5eabd9dcd074f3..6c7e2acd9de39c 100644 --- a/ee/app/finders/ee/ci/jobs_finder.rb +++ b/ee/app/finders/ee/ci/jobs_finder.rb @@ -7,11 +7,9 @@ module JobsFinder private - override :filter_by_failure_reason - def filter_by_failure_reason(builds) - return super unless use_failure_reason_filter? && License.feature_available?(:runner_performance_insights) - - builds.recently_failed_on_instance_runner(params[:failure_reason]) # currently limited to instance runners + override :filter_builds + def filter_builds(builds) + filter_by_failure_reason(super) end override :use_runner_type_filter? @@ -23,7 +21,15 @@ def use_runner_type_filter? super end + def filter_by_failure_reason(builds) + return builds unless use_failure_reason_filter? + + builds.recently_failed_on_instance_runner(params[:failure_reason]) # currently limited to instance runners + end + def use_failure_reason_filter? + return false unless License.feature_available?(:runner_performance_insights) + params[:failure_reason].present? && params[:runner_type] == ['instance_type'] end end diff --git a/ee/app/graphql/ee/resolvers/ci/all_jobs_resolver.rb b/ee/app/graphql/ee/resolvers/ci/all_jobs_resolver.rb index abb58ddbf2307e..c4861655a996d6 100644 --- a/ee/app/graphql/ee/resolvers/ci/all_jobs_resolver.rb +++ b/ee/app/graphql/ee/resolvers/ci/all_jobs_resolver.rb @@ -33,8 +33,8 @@ def ready?(failure_reason: nil, runner_types: nil, **_args) private - override :params_from - def params_from(args) + override :params_data + def params_data(args) super.merge(failure_reason: args[:failure_reason]) end end diff --git a/ee/spec/finders/ee/ci/jobs_finder_spec.rb b/ee/spec/finders/ee/ci/jobs_finder_spec.rb index eee151c7f33022..b1c00a3564dbac 100644 --- a/ee/spec/finders/ee/ci/jobs_finder_spec.rb +++ b/ee/spec/finders/ee/ci/jobs_finder_spec.rb @@ -47,15 +47,19 @@ context 'without runner_performance_insights license' do let(:runner_performance_insights) { false } - it 'returns empty collection' do - is_expected.to be_empty + it 'ignores :failure_reason argument, returning all jobs' do + expect(::Ci::Build).not_to receive(:recently_failed_on_instance_runner) + + expect(finder_execute.ids).to match_array Ci::Build.all.ids end context 'with param :runner_type set to [instance_type]' do let(:params) { { failure_reason: :runner_system_failure, runner_type: %w[instance_type] } } - it 'returns empty collection' do - is_expected.to be_empty + it 'ignores :failure_reason argument, returning instance runner jobs' do + expect(::Ci::Build).not_to receive(:recently_failed_on_instance_runner) + + expect(finder_execute.ids).to contain_exactly(job1.id, job2.id, job7.id) end end end @@ -77,10 +81,10 @@ context 'with param :runner_type set to multiple runner types', :aggregate_failures do let(:params) { { failure_reason: :runner_system_failure, runner_type: %w[instance_type group_type] } } - it 'returns empty collection' do + it 'ignores :failure_reason argument, returning instance and group runner jobs' do expect(::Ci::Build).not_to receive(:recently_failed_on_instance_runner) - expect(finder_execute.ids).to be_empty + expect(finder_execute.ids).to contain_exactly(job1.id, job2.id, job3.id, job5.id, job7.id) end context 'with feature flag :admin_jobs_filter_runner_type disabled' do @@ -88,10 +92,10 @@ stub_feature_flags(admin_jobs_filter_runner_type: false) end - it 'returns empty collection' do + it 'ignores :runner_type argument, returning all jobs' do expect(::Ci::Build).not_to receive(:recently_failed_on_instance_runner) - expect(finder_execute.ids).to be_empty + expect(finder_execute.ids).to match_array Ci::Build.all.ids end end end diff --git a/ee/spec/graphql/ee/resolvers/ci/all_jobs_resolver_spec.rb b/ee/spec/graphql/ee/resolvers/ci/all_jobs_resolver_spec.rb deleted file mode 100644 index 6f7c84f429e8a7..00000000000000 --- a/ee/spec/graphql/ee/resolvers/ci/all_jobs_resolver_spec.rb +++ /dev/null @@ -1,120 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe Resolvers::Ci::AllJobsResolver, feature_category: :continuous_integration do - include GraphqlHelpers - - let_it_be(:instance_runner) { create(:ci_runner, :instance) } - let_it_be(:successful_job) { create(:ci_build, :success, name: 'successful_job') } - let_it_be(:failed_job) { create(:ci_build, :failed, name: 'failed_job') } - let_it_be(:pending_job) { create(:ci_build, :pending, name: 'pending_job') } - let_it_be(:system_failure_job) do - create(:ci_build, :failed, failure_reason: :runner_system_failure, runner: instance_runner, - name: 'system_failure_job') - end - - let(:args) { {} } - - describe '#resolve' do - subject(:request) { resolve_jobs(args) } - - context 'when current user is an admin' do - let_it_be(:current_user) { create(:admin) } - - shared_examples 'executes as admin' do - context "with argument `failure_reason`", feature_category: :runner_fleet do - let(:args) { { failure_reason: failure_reason } } - - before_all do - project = create(:project) - project_runner = create(:ci_runner, :project, projects: [project]) - create(:ci_build, :failed, failure_reason: :runner_system_failure, runner: project_runner, - name: 'system_failure_job2') - end - - before do - stub_licensed_features(runner_performance_insights: true) - - # Mimic the transition from running to success/failed state - Ci::Build.all.each { |job| Ci::BuildFinishedWorker.new.perform(job.id) } - end - - context 'as RUNNER_SYSTEM_FAILURE' do - let(:failure_reason) { 'RUNNER_SYSTEM_FAILURE' } - - it 'generates an error' do - expect_graphql_error_to_be_created(Gitlab::Graphql::Errors::ArgumentError, - 'failureReason can only be used together with runnerTypes: INSTANCE_TYPE' - ) do - request - end - end - - context 'with argument `runnerTypes`' do - let(:args) { { runner_types: runner_types, failure_reason: failure_reason } } - - context 'as INSTANCE_TYPE' do - let(:runner_types) { ['INSTANCE_TYPE'] } - - it { is_expected.to contain_exactly(system_failure_job) } - end - - context 'as GROUP_TYPE' do - let(:runner_types) { ['GROUP_TYPE'] } - - it 'generates an error' do - expect_graphql_error_to_be_created(Gitlab::Graphql::Errors::ArgumentError, - 'failureReason can only be used together with runnerTypes: INSTANCE_TYPE' - ) do - request - end - end - end - end - end - - context 'as RUNNER_UNSUPPORTED' do - let(:failure_reason) { 'RUNNER_UNSUPPORTED' } - - context 'with argument `runnerTypes`' do - let(:args) { { runner_types: runner_types, failure_reason: failure_reason } } - - context 'as INSTANCE_TYPE' do - let(:runner_types) { ['INSTANCE_TYPE'] } - - it 'generates an error' do - expect_graphql_error_to_be_created( - Gitlab::Graphql::Errors::ArgumentError, 'failureReason only supports RUNNER_SYSTEM_FAILURE' - ) do - request - end - end - end - end - end - end - end - - context 'when admin mode setting is disabled', :do_not_mock_admin_mode_setting do - it_behaves_like 'executes as admin' - end - - context 'when admin mode setting is enabled' do - context 'when in admin mode', :enable_admin_mode do - it_behaves_like 'executes as admin' - end - - context 'when not in admin mode' do - it { is_expected.to be_empty } - end - end - end - end - - private - - def resolve_jobs(args = {}, context = { current_user: current_user }) - resolve(described_class, args: args, ctx: context) - end -end diff --git a/ee/spec/requests/api/graphql/ci/jobs_spec.rb b/ee/spec/requests/api/graphql/ci/jobs_spec.rb new file mode 100644 index 00000000000000..c9e5ae4809d124 --- /dev/null +++ b/ee/spec/requests/api/graphql/ci/jobs_spec.rb @@ -0,0 +1,115 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe 'Query.jobs', feature_category: :continuous_integration do + include GraphqlHelpers + + let_it_be(:instance_runner) { create(:ci_runner, :instance) } + + let_it_be(:successful_job) { create(:ci_build, :success, name: 'successful_job') } + let_it_be(:failed_job) { create(:ci_build, :failed, name: 'failed_job') } + let_it_be(:pending_job) { create(:ci_build, :pending, name: 'pending_job') } + let_it_be(:system_failure_job) do + create(:ci_build, :failed, failure_reason: :runner_system_failure, runner: instance_runner, + name: 'system_failure_job') + end + + let(:query_path) do + [ + [:jobs, query_jobs_args], + [:nodes] + ] + end + + let(:query) do + wrap_fields(query_graphql_path(query_path, 'id')) + end + + let(:jobs_graphql_data) { graphql_data_at(:jobs, :nodes) } + + subject(:request) { post_graphql(query, current_user: current_user) } + + context 'when current user is an admin' do + let_it_be(:current_user) { create(:admin) } + + context "with argument `failure_reason`", feature_category: :runner_fleet do + let(:query_jobs_args) do + graphql_args(failure_reason: failure_reason) + end + + before_all do + project = create(:project) + project_runner = create(:ci_runner, :project, projects: [project]) + create(:ci_build, :failed, failure_reason: :runner_system_failure, runner: project_runner, + name: 'system_failure_job2') + end + + before do + stub_licensed_features(runner_performance_insights: true) + + # Mimic the transition from running to success/failed state + Ci::Build.all.each { |job| Ci::BuildFinishedWorker.new.perform(job.id) } + end + + context 'as RUNNER_SYSTEM_FAILURE' do + let(:failure_reason) { :RUNNER_SYSTEM_FAILURE } + + it 'generates an error' do + request + + expect_graphql_errors_to_include 'failureReason can only be used together with runnerTypes: INSTANCE_TYPE' + end + + context 'with argument `runnerTypes`' do + let(:query_jobs_args) do + graphql_args(runner_types: runner_types, failure_reason: failure_reason) + end + + context 'as INSTANCE_TYPE' do + let(:runner_types) { [:INSTANCE_TYPE] } + + it_behaves_like 'a working graphql query that returns data' do + before do + request + end + + it { expect(jobs_graphql_data).to contain_exactly(a_graphql_entity_for(system_failure_job)) } + end + end + + context 'as GROUP_TYPE' do + let(:runner_types) { [:GROUP_TYPE] } + + it 'generates an error' do + request + + expect_graphql_errors_to_include 'failureReason can only be used together with ' \ + 'runnerTypes: INSTANCE_TYPE' + end + end + end + end + + context 'as RUNNER_UNSUPPORTED' do + let(:failure_reason) { :RUNNER_UNSUPPORTED } + + context 'with argument `runnerTypes`' do + let(:query_jobs_args) do + graphql_args(runner_types: runner_types, failure_reason: failure_reason) + end + + context 'as INSTANCE_TYPE' do + let(:runner_types) { [:INSTANCE_TYPE] } + + it 'generates an error' do + request + + expect_graphql_errors_to_include 'failureReason only supports RUNNER_SYSTEM_FAILURE' + end + end + end + end + end + end +end -- GitLab From 3c90d9a98b3c6d1e698649c04a1fc28caa3b439d Mon Sep 17 00:00:00 2001 From: Pedro Pombeiro Date: Wed, 6 Sep 2023 12:07:22 +0200 Subject: [PATCH 07/14] Add failure_reason filter to InstanceRunnerFailedJobs --- .../models/ci/instance_runner_failed_jobs.rb | 9 +- ee/app/models/ee/ci/build.rb | 3 +- .../ci/instance_runner_failed_jobs_spec.rb | 90 ++++++++++++------- 3 files changed, 64 insertions(+), 38 deletions(-) diff --git a/ee/app/models/ci/instance_runner_failed_jobs.rb b/ee/app/models/ci/instance_runner_failed_jobs.rb index b7e8c6a4fd6cae..dc719ca38737a0 100644 --- a/ee/app/models/ci/instance_runner_failed_jobs.rb +++ b/ee/app/models/ci/instance_runner_failed_jobs.rb @@ -5,6 +5,7 @@ class InstanceRunnerFailedJobs # Safety margin for situations where there is a mismatch between the async insert order and the finished_at value JOB_LIMIT_MARGIN = 10 JOB_LIMIT = 100 + SUPPORTED_FAILURE_REASONS = %i[runner_system_failure].freeze class << self def track(build) @@ -18,7 +19,11 @@ def track(build) end end - def recent_jobs + def recent_jobs(failure_reason:) + unless SUPPORTED_FAILURE_REASONS.include?(failure_reason.to_sym) + raise ArgumentError, "The only failure reason(s) supported are #{SUPPORTED_FAILURE_REASONS.join(', ')}" + end + return Ci::Build.none unless License.feature_available?(:runner_performance_insights) job_ids = with_redis do |redis| @@ -45,7 +50,7 @@ def max_admissible_job_count def track_job?(build) License.feature_available?(:runner_performance_insights) && - build.failure_reason == 'runner_system_failure' && + SUPPORTED_FAILURE_REASONS.include?(build.failure_reason.to_sym) && build.runner.instance_type? end end diff --git a/ee/app/models/ee/ci/build.rb b/ee/app/models/ee/ci/build.rb index 5bae8abfec2c42..867a541e7e1d8c 100644 --- a/ee/app/models/ee/ci/build.rb +++ b/ee/app/models/ee/ci/build.rb @@ -54,8 +54,7 @@ module Build scope :recently_failed_on_instance_runner, -> (failure_reason) do reorder(nil) # Remove any default ordering as we rely on order from Ci::InstanceRunnerFailedJobs.recent_jobs - .merge(::Ci::InstanceRunnerFailedJobs.recent_jobs) - .where(failure_reason: failure_reason) + .merge(::Ci::InstanceRunnerFailedJobs.recent_jobs(failure_reason: failure_reason)) end state_machine :status do diff --git a/ee/spec/models/ci/instance_runner_failed_jobs_spec.rb b/ee/spec/models/ci/instance_runner_failed_jobs_spec.rb index 99df1913b090f8..54ebec167d2f0b 100644 --- a/ee/spec/models/ci/instance_runner_failed_jobs_spec.rb +++ b/ee/spec/models/ci/instance_runner_failed_jobs_spec.rb @@ -66,7 +66,11 @@ end describe '.recent_jobs' do - subject(:recent_jobs) { described_class.recent_jobs } + def recent_jobs + described_class.recent_jobs(failure_reason: failure_reason) + end + + subject(:scope) { recent_jobs } let_it_be(:runner) { create(:ci_runner, :instance) } let_it_be(:job_args) { { runner: runner, failure_reason: :runner_system_failure } } @@ -77,57 +81,71 @@ context 'with runner_performance_insights licensed feature' do let(:runner_performance_insights) { true } - context 'when content is not set' do - it { is_expected.to be_empty } - end + context 'when failure_reason is not runner_system_failure' do + let(:failure_reason) { :runner_unsupported } - context 'when jobs are added' do - before do - described_class.track(job) + it 'raises an error' do + expect { recent_jobs }.to raise_error( + ArgumentError, 'The only failure reason(s) supported are runner_system_failure' + ) end + end - it 'returns 3 most recently finished jobs' do - expect(described_class.recent_jobs).to contain_exactly(an_object_having_attributes(id: job.id)) - - described_class.track(job2) - described_class.track(job3) + context 'when failure_reason is runner_system_failure' do + let(:failure_reason) { :runner_system_failure } - expect(described_class.recent_jobs).to match([ - an_object_having_attributes(id: job.id), - an_object_having_attributes(id: job2.id), - an_object_having_attributes(id: job3.id) - ]) + context 'when content is not set' do + it { is_expected.to be_empty } end - context 'when jobs are added in different order' do + context 'when jobs are added' do + before do + described_class.track(job) + end + it 'returns 3 most recently finished jobs' do - expect(described_class.recent_jobs).to contain_exactly(an_object_having_attributes(id: job.id)) + expect(recent_jobs).to contain_exactly(an_object_having_attributes(id: job.id)) - described_class.track(job3) described_class.track(job2) + described_class.track(job3) - expect(described_class.recent_jobs).to match([ + expect(recent_jobs).to match([ an_object_having_attributes(id: job.id), an_object_having_attributes(id: job2.id), an_object_having_attributes(id: job3.id) ]) end - end - context 'when trimming is required' do - before do - stub_const("#{described_class}::JOB_LIMIT", 1) - stub_const("#{described_class}::JOB_LIMIT_MARGIN", 1) + context 'when jobs are added in different order' do + it 'returns 3 most recently finished jobs' do + expect(recent_jobs).to contain_exactly(an_object_having_attributes(id: job.id)) + + described_class.track(job3) + described_class.track(job2) + + expect(recent_jobs).to match([ + an_object_having_attributes(id: job.id), + an_object_having_attributes(id: job2.id), + an_object_having_attributes(id: job3.id) + ]) + end end - it 'returns 2 most recently finished jobs and purges the rest', :aggregate_failures do - described_class.track(job3) - described_class.track(job2) + context 'when trimming is required' do + before do + stub_const("#{described_class}::JOB_LIMIT", 1) + stub_const("#{described_class}::JOB_LIMIT_MARGIN", 1) + end + + it 'returns 2 most recently finished jobs and purges the rest', :aggregate_failures do + described_class.track(job3) + described_class.track(job2) - # Only the last 2 jobs saved will be retained - expect(redis_stored_job_ids).to eq(formatted_job_ids_for(job2, job3)) - # and of those 2, the most recently finished will be returned (JOB_LIMIT) - expect(described_class.recent_jobs).to contain_exactly(an_object_having_attributes(id: job2.id)) + # Only the last 2 jobs saved will be retained + expect(redis_stored_job_ids).to eq(formatted_job_ids_for(job2, job3)) + # and of those 2, the most recently finished will be returned (JOB_LIMIT) + is_expected.to contain_exactly(an_object_having_attributes(id: job2.id)) + end end end end @@ -136,7 +154,11 @@ context 'without runner_performance_insights licensed feature' do let(:runner_performance_insights) { false } - it { is_expected.to be_empty } + context 'when failure_reason is runner_system_failure' do + let(:failure_reason) { :runner_system_failure } + + it { is_expected.to be_empty } + end end end -- GitLab From e6cb3fe83a9ef5a299e53430652925c80470dd82 Mon Sep 17 00:00:00 2001 From: Pedro Pombeiro Date: Wed, 6 Sep 2023 12:13:40 +0200 Subject: [PATCH 08/14] Address MR review comment --- ee/spec/requests/api/graphql/ci/jobs_spec.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/ee/spec/requests/api/graphql/ci/jobs_spec.rb b/ee/spec/requests/api/graphql/ci/jobs_spec.rb index c9e5ae4809d124..8529a9044b4881 100644 --- a/ee/spec/requests/api/graphql/ci/jobs_spec.rb +++ b/ee/spec/requests/api/graphql/ci/jobs_spec.rb @@ -38,9 +38,9 @@ graphql_args(failure_reason: failure_reason) end - before_all do - project = create(:project) - project_runner = create(:ci_runner, :project, projects: [project]) + let_it_be(:_system_failure_on_project_runner) do + project_runner = create(:ci_runner, :project) + create(:ci_build, :failed, failure_reason: :runner_system_failure, runner: project_runner, name: 'system_failure_job2') end -- GitLab From dd837aaa0eb6b3427bfb0ae126ee914e568281a5 Mon Sep 17 00:00:00 2001 From: Pedro Pombeiro Date: Wed, 6 Sep 2023 17:25:49 +0200 Subject: [PATCH 09/14] Address Fabio's MR review comments --- ee/app/models/ci/instance_runner_failed_jobs.rb | 5 ++++- ee/spec/requests/api/graphql/ci/jobs_spec.rb | 3 +-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/ee/app/models/ci/instance_runner_failed_jobs.rb b/ee/app/models/ci/instance_runner_failed_jobs.rb index dc719ca38737a0..ab8074ff89fc72 100644 --- a/ee/app/models/ci/instance_runner_failed_jobs.rb +++ b/ee/app/models/ci/instance_runner_failed_jobs.rb @@ -31,7 +31,10 @@ def recent_jobs(failure_reason:) redis.lrange(key, 0, max_admissible_job_count - 1) end - Ci::Build.id_in(job_ids).order(finished_at: :desc, id: :desc).limit(JOB_LIMIT) + Ci::Build.id_in(job_ids) + .where(failure_reason: failure_reason) + .order(finished_at: :desc, id: :desc) + .limit(JOB_LIMIT) end private diff --git a/ee/spec/requests/api/graphql/ci/jobs_spec.rb b/ee/spec/requests/api/graphql/ci/jobs_spec.rb index 8529a9044b4881..a902e7e777c247 100644 --- a/ee/spec/requests/api/graphql/ci/jobs_spec.rb +++ b/ee/spec/requests/api/graphql/ci/jobs_spec.rb @@ -48,8 +48,7 @@ before do stub_licensed_features(runner_performance_insights: true) - # Mimic the transition from running to success/failed state - Ci::Build.all.each { |job| Ci::BuildFinishedWorker.new.perform(job.id) } + Ci::Build.all.each { |build| ::Ci::InstanceRunnerFailedJobs.track(build) } end context 'as RUNNER_SYSTEM_FAILURE' do -- GitLab From 1b5af8ec4dd1ba8c5b08e9c228b22dc2ba70a40d Mon Sep 17 00:00:00 2001 From: Pedro Pombeiro Date: Wed, 6 Sep 2023 17:44:19 +0200 Subject: [PATCH 10/14] Remove license check from JobsFinder --- ee/app/finders/ee/ci/jobs_finder.rb | 2 -- ee/app/models/ci/instance_runner_failed_jobs.rb | 2 +- ee/spec/finders/ee/ci/jobs_finder_spec.rb | 7 ++++--- ee/spec/models/ci/instance_runner_failed_jobs_spec.rb | 2 +- 4 files changed, 6 insertions(+), 7 deletions(-) diff --git a/ee/app/finders/ee/ci/jobs_finder.rb b/ee/app/finders/ee/ci/jobs_finder.rb index 6c7e2acd9de39c..69c20a7a78440a 100644 --- a/ee/app/finders/ee/ci/jobs_finder.rb +++ b/ee/app/finders/ee/ci/jobs_finder.rb @@ -28,8 +28,6 @@ def filter_by_failure_reason(builds) end def use_failure_reason_filter? - return false unless License.feature_available?(:runner_performance_insights) - params[:failure_reason].present? && params[:runner_type] == ['instance_type'] end end diff --git a/ee/app/models/ci/instance_runner_failed_jobs.rb b/ee/app/models/ci/instance_runner_failed_jobs.rb index ab8074ff89fc72..6e17fc8c5a7c8c 100644 --- a/ee/app/models/ci/instance_runner_failed_jobs.rb +++ b/ee/app/models/ci/instance_runner_failed_jobs.rb @@ -24,7 +24,7 @@ def recent_jobs(failure_reason:) raise ArgumentError, "The only failure reason(s) supported are #{SUPPORTED_FAILURE_REASONS.join(', ')}" end - return Ci::Build.none unless License.feature_available?(:runner_performance_insights) + return Ci::Build.all unless License.feature_available?(:runner_performance_insights) job_ids = with_redis do |redis| # Fetch a few more jobs in case there is a mismatch between the async insert order and the finished_at value diff --git a/ee/spec/finders/ee/ci/jobs_finder_spec.rb b/ee/spec/finders/ee/ci/jobs_finder_spec.rb index b1c00a3564dbac..0bd8bd191571d3 100644 --- a/ee/spec/finders/ee/ci/jobs_finder_spec.rb +++ b/ee/spec/finders/ee/ci/jobs_finder_spec.rb @@ -56,10 +56,11 @@ context 'with param :runner_type set to [instance_type]' do let(:params) { { failure_reason: :runner_system_failure, runner_type: %w[instance_type] } } - it 'ignores :failure_reason argument, returning instance runner jobs' do - expect(::Ci::Build).not_to receive(:recently_failed_on_instance_runner) + it 'ignores :failure_reason and :runner_type arguments, returning all runner jobs' do + expect(::Ci::Build).to receive(:recently_failed_on_instance_runner) + .with(:runner_system_failure).once.and_call_original - expect(finder_execute.ids).to contain_exactly(job1.id, job2.id, job7.id) + expect(finder_execute.ids).to match_array Ci::Build.all.ids end end end diff --git a/ee/spec/models/ci/instance_runner_failed_jobs_spec.rb b/ee/spec/models/ci/instance_runner_failed_jobs_spec.rb index 54ebec167d2f0b..415cdfeadcbcdc 100644 --- a/ee/spec/models/ci/instance_runner_failed_jobs_spec.rb +++ b/ee/spec/models/ci/instance_runner_failed_jobs_spec.rb @@ -157,7 +157,7 @@ def recent_jobs context 'when failure_reason is runner_system_failure' do let(:failure_reason) { :runner_system_failure } - it { is_expected.to be_empty } + it { is_expected.to eq(Ci::Build.all) } end end end -- GitLab From b7ef03d336fd86b0c5c8ec0284124da6c06ffd90 Mon Sep 17 00:00:00 2001 From: Pedro Pombeiro Date: Wed, 6 Sep 2023 18:09:38 +0200 Subject: [PATCH 11/14] Move reorder to InstanceRunnerFailedJobs --- ee/app/models/ci/instance_runner_failed_jobs.rb | 3 ++- ee/app/models/ee/ci/build.rb | 3 +-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/ee/app/models/ci/instance_runner_failed_jobs.rb b/ee/app/models/ci/instance_runner_failed_jobs.rb index 6e17fc8c5a7c8c..e837059050d928 100644 --- a/ee/app/models/ci/instance_runner_failed_jobs.rb +++ b/ee/app/models/ci/instance_runner_failed_jobs.rb @@ -24,7 +24,7 @@ def recent_jobs(failure_reason:) raise ArgumentError, "The only failure reason(s) supported are #{SUPPORTED_FAILURE_REASONS.join(', ')}" end - return Ci::Build.all unless License.feature_available?(:runner_performance_insights) + return Ci::Build.all.reorder(nil) unless License.feature_available?(:runner_performance_insights) job_ids = with_redis do |redis| # Fetch a few more jobs in case there is a mismatch between the async insert order and the finished_at value @@ -32,6 +32,7 @@ def recent_jobs(failure_reason:) end Ci::Build.id_in(job_ids) + .reorder(nil) # Remove any default ordering as we rely on order from Ci::InstanceRunnerFailedJobs.recent_jobs .where(failure_reason: failure_reason) .order(finished_at: :desc, id: :desc) .limit(JOB_LIMIT) diff --git a/ee/app/models/ee/ci/build.rb b/ee/app/models/ee/ci/build.rb index 867a541e7e1d8c..32131bc1e3b726 100644 --- a/ee/app/models/ee/ci/build.rb +++ b/ee/app/models/ee/ci/build.rb @@ -53,8 +53,7 @@ module Build end scope :recently_failed_on_instance_runner, -> (failure_reason) do - reorder(nil) # Remove any default ordering as we rely on order from Ci::InstanceRunnerFailedJobs.recent_jobs - .merge(::Ci::InstanceRunnerFailedJobs.recent_jobs(failure_reason: failure_reason)) + merge(::Ci::InstanceRunnerFailedJobs.recent_jobs(failure_reason: failure_reason)) end state_machine :status do -- GitLab From 6d7c3fe3650542fb99ffeec7838e2226837cb0a5 Mon Sep 17 00:00:00 2001 From: Pedro Pombeiro Date: Wed, 6 Sep 2023 18:23:23 +0200 Subject: [PATCH 12/14] Raise error in JobsFinder --- ee/app/finders/ee/ci/jobs_finder.rb | 17 +++- .../models/ci/instance_runner_failed_jobs.rb | 26 ++++-- ee/spec/finders/ee/ci/jobs_finder_spec.rb | 92 ++++++++++++------- .../ci/instance_runner_failed_jobs_spec.rb | 2 +- 4 files changed, 92 insertions(+), 45 deletions(-) diff --git a/ee/app/finders/ee/ci/jobs_finder.rb b/ee/app/finders/ee/ci/jobs_finder.rb index 69c20a7a78440a..377bdc82adee10 100644 --- a/ee/app/finders/ee/ci/jobs_finder.rb +++ b/ee/app/finders/ee/ci/jobs_finder.rb @@ -28,7 +28,22 @@ def filter_by_failure_reason(builds) end def use_failure_reason_filter? - params[:failure_reason].present? && params[:runner_type] == ['instance_type'] + failure_reason = params[:failure_reason] + runner_type = params[:runner_type] + + if failure_reason.present? + unless failure_reason == :runner_system_failure + raise ArgumentError, 'failure_reason only supports runner_system_failure' + end + + unless runner_type == %w[instance_type] + raise ArgumentError, 'failure_reason can only be used together with runner_type: instance_type' + end + + return true + end + + false end end end diff --git a/ee/app/models/ci/instance_runner_failed_jobs.rb b/ee/app/models/ci/instance_runner_failed_jobs.rb index e837059050d928..dbd7f4659d8c29 100644 --- a/ee/app/models/ci/instance_runner_failed_jobs.rb +++ b/ee/app/models/ci/instance_runner_failed_jobs.rb @@ -24,18 +24,24 @@ def recent_jobs(failure_reason:) raise ArgumentError, "The only failure reason(s) supported are #{SUPPORTED_FAILURE_REASONS.join(', ')}" end - return Ci::Build.all.reorder(nil) unless License.feature_available?(:runner_performance_insights) + jobs = + if License.feature_available?(:runner_performance_insights) + job_ids = with_redis do |redis| + # Fetch a few more jobs in case there is a mismatch between the async insert order and + # the finished_at value + redis.lrange(key, 0, max_admissible_job_count - 1) + end - job_ids = with_redis do |redis| - # Fetch a few more jobs in case there is a mismatch between the async insert order and the finished_at value - redis.lrange(key, 0, max_admissible_job_count - 1) - end + Ci::Build.id_in(job_ids) + .where(failure_reason: failure_reason) + .order(finished_at: :desc, id: :desc) + .limit(JOB_LIMIT) + else + Ci::Build.none + end - Ci::Build.id_in(job_ids) - .reorder(nil) # Remove any default ordering as we rely on order from Ci::InstanceRunnerFailedJobs.recent_jobs - .where(failure_reason: failure_reason) - .order(finished_at: :desc, id: :desc) - .limit(JOB_LIMIT) + # Remove any default ordering as we rely on order from Ci::InstanceRunnerFailedJobs.recent_jobs + jobs.reorder(nil) end private diff --git a/ee/spec/finders/ee/ci/jobs_finder_spec.rb b/ee/spec/finders/ee/ci/jobs_finder_spec.rb index 0bd8bd191571d3..c4d0307c5f352b 100644 --- a/ee/spec/finders/ee/ci/jobs_finder_spec.rb +++ b/ee/spec/finders/ee/ci/jobs_finder_spec.rb @@ -14,53 +14,55 @@ let(:current_user) { admin } context 'when admin mode is enabled', :enable_admin_mode do - context 'with param `failure_reason` set to :runner_system_failure', :clean_gitlab_redis_shared_state, - feature_category: :runner_fleet do - let(:params) { { failure_reason: :runner_system_failure } } + let_it_be(:instance_runner) { create(:ci_runner, :instance) } + let_it_be(:group_runner) { create(:ci_runner, :group) } + let_it_be(:project_runner) { create(:ci_runner, :project) } + let_it_be(:job_args) { { runner: instance_runner, failure_reason: :runner_system_failure } } + let_it_be(:job1) { create(:ci_build, :failed, finished_at: 1.minute.ago, **job_args) } + let_it_be(:job2) { create(:ci_build, :failed, finished_at: 2.minutes.ago, **job_args) } + let_it_be(:job7) { create(:ci_build, :failed, finished_at: 2.minutes.ago, **job_args) } + let_it_be(:job3) { create(:ci_build, :failed, finished_at: 2.minutes.ago, runner: group_runner) } + let_it_be(:job4) { create(:ci_build, :failed, finished_at: 2.minutes.ago, runner: project_runner) } + let_it_be(:job5) do + create(:ci_build, :failed, finished_at: 2.minutes.ago, runner: group_runner, + failure_reason: :runner_system_failure) + end - let_it_be(:instance_runner) { create(:ci_runner, :instance) } - let_it_be(:group_runner) { create(:ci_runner, :group) } - let_it_be(:project_runner) { create(:ci_runner, :project) } - let_it_be(:job_args) { { runner: instance_runner, failure_reason: :runner_system_failure } } - let_it_be(:job1) { create(:ci_build, :failed, finished_at: 1.minute.ago, **job_args) } - let_it_be(:job2) { create(:ci_build, :failed, finished_at: 2.minutes.ago, **job_args) } - let_it_be(:job7) { create(:ci_build, :failed, finished_at: 2.minutes.ago, **job_args) } - let_it_be(:job3) { create(:ci_build, :failed, finished_at: 2.minutes.ago, runner: group_runner) } - let_it_be(:job4) { create(:ci_build, :failed, finished_at: 2.minutes.ago, runner: project_runner) } - let_it_be(:job5) do - create(:ci_build, :failed, finished_at: 2.minutes.ago, runner: group_runner, - failure_reason: :runner_system_failure) - end + let_it_be(:job6) do + create(:ci_build, :failed, finished_at: 2.minutes.ago, runner: project_runner, + failure_reason: :runner_system_failure) + end - let_it_be(:job6) do - create(:ci_build, :failed, finished_at: 2.minutes.ago, runner: project_runner, - failure_reason: :runner_system_failure) - end + before do + stub_licensed_features(runner_performance_insights: runner_performance_insights) - before do - stub_licensed_features(runner_performance_insights: runner_performance_insights) + ::Ci::InstanceRunnerFailedJobs.track(job1) + ::Ci::InstanceRunnerFailedJobs.track(job2) + end - ::Ci::InstanceRunnerFailedJobs.track(job1) - ::Ci::InstanceRunnerFailedJobs.track(job2) - end + context 'with param `failure_reason` set to :runner_system_failure', :clean_gitlab_redis_shared_state, + feature_category: :runner_fleet do + let(:params) { { failure_reason: :runner_system_failure } } context 'without runner_performance_insights license' do let(:runner_performance_insights) { false } - it 'ignores :failure_reason argument, returning all jobs' do + it 'raises ArgumentError due to lack of runner_type' do expect(::Ci::Build).not_to receive(:recently_failed_on_instance_runner) - expect(finder_execute.ids).to match_array Ci::Build.all.ids + expect { finder_execute }.to raise_error( + ArgumentError, 'failure_reason can only be used together with runner_type: instance_type' + ) end context 'with param :runner_type set to [instance_type]' do let(:params) { { failure_reason: :runner_system_failure, runner_type: %w[instance_type] } } - it 'ignores :failure_reason and :runner_type arguments, returning all runner jobs' do + it 'returns no jobs due to lack of license' do expect(::Ci::Build).to receive(:recently_failed_on_instance_runner) .with(:runner_system_failure).once.and_call_original - expect(finder_execute.ids).to match_array Ci::Build.all.ids + expect(finder_execute.ids).to be_empty end end end @@ -82,10 +84,12 @@ context 'with param :runner_type set to multiple runner types', :aggregate_failures do let(:params) { { failure_reason: :runner_system_failure, runner_type: %w[instance_type group_type] } } - it 'ignores :failure_reason argument, returning instance and group runner jobs' do + it 'raises ArgumentError due to multiple runner types' do expect(::Ci::Build).not_to receive(:recently_failed_on_instance_runner) - expect(finder_execute.ids).to contain_exactly(job1.id, job2.id, job3.id, job5.id, job7.id) + expect { finder_execute }.to raise_error( + ArgumentError, 'failure_reason can only be used together with runner_type: instance_type' + ) end context 'with feature flag :admin_jobs_filter_runner_type disabled' do @@ -93,15 +97,37 @@ stub_feature_flags(admin_jobs_filter_runner_type: false) end - it 'ignores :runner_type argument, returning all jobs' do + it 'raises ArgumentError due to multiple runner types' do expect(::Ci::Build).not_to receive(:recently_failed_on_instance_runner) - expect(finder_execute.ids).to match_array Ci::Build.all.ids + expect { finder_execute }.to raise_error( + ArgumentError, 'failure_reason can only be used together with runner_type: instance_type' + ) end end end end end + + context 'with param `failure_reason` not set to :runner_system_failure', :clean_gitlab_redis_shared_state, + feature_category: :runner_fleet do + let(:params) { { failure_reason: :runner_unsupported } } + + context 'with runner_performance_insights license' do + let(:runner_performance_insights) { true } + + context 'with param :runner_type set to [instance_type]' do + let(:params) { { failure_reason: :runner_unsupported, runner_type: %w[instance_type] } } + + it 'raises ArgumentError due to unsupported failure_reason' do + expect(::Ci::Build).not_to receive(:recently_failed_on_instance_runner) + + expect { finder_execute } + .to raise_error(ArgumentError, 'failure_reason only supports runner_system_failure') + end + end + end + end end end end diff --git a/ee/spec/models/ci/instance_runner_failed_jobs_spec.rb b/ee/spec/models/ci/instance_runner_failed_jobs_spec.rb index 415cdfeadcbcdc..54ebec167d2f0b 100644 --- a/ee/spec/models/ci/instance_runner_failed_jobs_spec.rb +++ b/ee/spec/models/ci/instance_runner_failed_jobs_spec.rb @@ -157,7 +157,7 @@ def recent_jobs context 'when failure_reason is runner_system_failure' do let(:failure_reason) { :runner_system_failure } - it { is_expected.to eq(Ci::Build.all) } + it { is_expected.to be_empty } end end end -- GitLab From 7d67fd06f75fb334e18d3395fea1a380e25e3b83 Mon Sep 17 00:00:00 2001 From: Pedro Pombeiro Date: Thu, 7 Sep 2023 17:41:45 +0200 Subject: [PATCH 13/14] Remove condition checks from ready? --- .../ee/resolvers/ci/all_jobs_resolver.rb | 17 ++++------------- ee/spec/requests/api/graphql/ci/jobs_spec.rb | 15 ++------------- 2 files changed, 6 insertions(+), 26 deletions(-) diff --git a/ee/app/graphql/ee/resolvers/ci/all_jobs_resolver.rb b/ee/app/graphql/ee/resolvers/ci/all_jobs_resolver.rb index c4861655a996d6..09694a7e938de4 100644 --- a/ee/app/graphql/ee/resolvers/ci/all_jobs_resolver.rb +++ b/ee/app/graphql/ee/resolvers/ci/all_jobs_resolver.rb @@ -15,20 +15,11 @@ module AllJobsResolver '`runnerTypes: INSTANCE_TYPE` is supported.' end - override :ready? - def ready?(failure_reason: nil, runner_types: nil, **_args) - if failure_reason.present? - if failure_reason != :runner_system_failure - raise ::Gitlab::Graphql::Errors::ArgumentError, 'failureReason only supports RUNNER_SYSTEM_FAILURE' - end - - if runner_types != %w[instance_type] - raise ::Gitlab::Graphql::Errors::ArgumentError, - 'failureReason can only be used together with runnerTypes: INSTANCE_TYPE' - end - end - + override :resolve_with_lookahead + def resolve_with_lookahead(**args) super + rescue ArgumentError => e + raise ::Gitlab::Graphql::Errors::ArgumentError, e.message end private diff --git a/ee/spec/requests/api/graphql/ci/jobs_spec.rb b/ee/spec/requests/api/graphql/ci/jobs_spec.rb index a902e7e777c247..4f30cf0159d772 100644 --- a/ee/spec/requests/api/graphql/ci/jobs_spec.rb +++ b/ee/spec/requests/api/graphql/ci/jobs_spec.rb @@ -57,7 +57,7 @@ it 'generates an error' do request - expect_graphql_errors_to_include 'failureReason can only be used together with runnerTypes: INSTANCE_TYPE' + expect_graphql_errors_to_include 'failure_reason can only be used together with runner_type: instance_type' end context 'with argument `runnerTypes`' do @@ -76,17 +76,6 @@ it { expect(jobs_graphql_data).to contain_exactly(a_graphql_entity_for(system_failure_job)) } end end - - context 'as GROUP_TYPE' do - let(:runner_types) { [:GROUP_TYPE] } - - it 'generates an error' do - request - - expect_graphql_errors_to_include 'failureReason can only be used together with ' \ - 'runnerTypes: INSTANCE_TYPE' - end - end end end @@ -104,7 +93,7 @@ it 'generates an error' do request - expect_graphql_errors_to_include 'failureReason only supports RUNNER_SYSTEM_FAILURE' + expect_graphql_errors_to_include 'failure_reason only supports runner_system_failure' end end end -- GitLab From 81f8f12eb1bafe381c8105f53c7d8e04bc2ce07d Mon Sep 17 00:00:00 2001 From: Pedro Pombeiro Date: Thu, 7 Sep 2023 17:52:03 +0200 Subject: [PATCH 14/14] Fix sort order --- .../models/ci/instance_runner_failed_jobs.rb | 26 +++++++------------ ee/spec/models/ci/build_spec.rb | 25 +++++++++--------- 2 files changed, 22 insertions(+), 29 deletions(-) diff --git a/ee/app/models/ci/instance_runner_failed_jobs.rb b/ee/app/models/ci/instance_runner_failed_jobs.rb index dbd7f4659d8c29..ab3fd09a7688d1 100644 --- a/ee/app/models/ci/instance_runner_failed_jobs.rb +++ b/ee/app/models/ci/instance_runner_failed_jobs.rb @@ -24,24 +24,18 @@ def recent_jobs(failure_reason:) raise ArgumentError, "The only failure reason(s) supported are #{SUPPORTED_FAILURE_REASONS.join(', ')}" end - jobs = - if License.feature_available?(:runner_performance_insights) - job_ids = with_redis do |redis| - # Fetch a few more jobs in case there is a mismatch between the async insert order and - # the finished_at value - redis.lrange(key, 0, max_admissible_job_count - 1) - end + return Ci::Build.none unless License.feature_available?(:runner_performance_insights) - Ci::Build.id_in(job_ids) - .where(failure_reason: failure_reason) - .order(finished_at: :desc, id: :desc) - .limit(JOB_LIMIT) - else - Ci::Build.none - end + job_ids = with_redis do |redis| + # Fetch a few more jobs in case there is a mismatch between the async insert order and + # the finished_at value + redis.lrange(key, 0, max_admissible_job_count - 1) + end - # Remove any default ordering as we rely on order from Ci::InstanceRunnerFailedJobs.recent_jobs - jobs.reorder(nil) + Ci::Build.id_in(job_ids) + .where(failure_reason: failure_reason) + .reorder(finished_at: :desc, id: :desc) + .limit(JOB_LIMIT) end private diff --git a/ee/spec/models/ci/build_spec.rb b/ee/spec/models/ci/build_spec.rb index 2f0ea5ac91eadc..0b1da54037b156 100644 --- a/ee/spec/models/ci/build_spec.rb +++ b/ee/spec/models/ci/build_spec.rb @@ -671,7 +671,9 @@ end describe '.recently_failed_on_instance_runner', :clean_gitlab_redis_shared_state, feature_category: :runner_fleet do - subject(:recently_failed_on_instance_runner) { scope(failure_reason) } + subject(:recently_failed_on_instance_runner) do + described_class.recently_failed_on_instance_runner(failure_reason) + end before do stub_licensed_features(runner_performance_insights: true) @@ -682,10 +684,6 @@ let_it_be(:job1) { create(:ci_build, :failed, finished_at: 1.minute.ago, **job_args) } let_it_be(:job2) { create(:ci_build, :failed, finished_at: 2.minutes.ago, **job_args) } - def scope(failure_reason, base_scope: described_class) - base_scope.recently_failed_on_instance_runner(failure_reason) - end - context 'with failure_reason set to :runner_system_failure' do let(:failure_reason) { :runner_system_failure } @@ -706,15 +704,16 @@ def scope(failure_reason, base_scope: described_class) ]) end - context 'with pre-existing order on scope' do - subject(:recently_failed_on_instance_runner) { scope(:runner_system_failure, base_scope: described_class.order(:id)) } + it 'overrides the order of returned builds' do + expect(described_class.order(id: :asc).recently_failed_on_instance_runner(failure_reason)).to match([ + an_object_having_attributes(id: job1.id), + an_object_having_attributes(id: job2.id) + ]) - it 'overrides the order of returned builds' do - is_expected.to match([ - an_object_having_attributes(id: job1.id), - an_object_having_attributes(id: job2.id) - ]) - end + expect(described_class.order(id: :desc).recently_failed_on_instance_runner(failure_reason)).to match([ + an_object_having_attributes(id: job1.id), + an_object_having_attributes(id: job2.id) + ]) end end end -- GitLab