From 90d39e74c0d7e66a7d0cb8a06351302506ade5ba Mon Sep 17 00:00:00 2001 From: Pedro Pombeiro Date: Thu, 24 Aug 2023 12:53:10 +0200 Subject: [PATCH 1/2] Create runner_failed_build records --- ee/app/models/ci/runner_failed_build.rb | 19 +++++ ee/app/models/ee/ci/build.rb | 8 ++ ee/spec/models/ci/build_spec.rb | 80 +++++++++++++++++++ ee/spec/models/ci/runner_failed_build_spec.rb | 12 +++ 4 files changed, 119 insertions(+) diff --git a/ee/app/models/ci/runner_failed_build.rb b/ee/app/models/ci/runner_failed_build.rb index 9f7e13991e7407..b3489faba55b98 100644 --- a/ee/app/models/ci/runner_failed_build.rb +++ b/ee/app/models/ci/runner_failed_build.rb @@ -13,5 +13,24 @@ class RunnerFailedBuild < Ci::ApplicationRecord validates :build, presence: true validates :finished_at, presence: true + + class << self + def upsert_from_build!(build) + entry = new(args_from_build(build)) + + entry.validate! + + upsert(entry.attributes.compact, returning: %w[build_id], unique_by: [:build_id, :partition_id]) + end + + private + + def args_from_build(build) + { + build: build, + finished_at: build.finished_at + } + end + end end end diff --git a/ee/app/models/ee/ci/build.rb b/ee/app/models/ee/ci/build.rb index b28c303c17d68c..a9a9f3b3b020c0 100644 --- a/ee/app/models/ee/ci/build.rb +++ b/ee/app/models/ee/ci/build.rb @@ -57,6 +57,14 @@ module Build ::Ci::Minutes::UpdateBuildMinutesService.new(build.project, nil).execute(build) end end + + after_transition running: :failed do |build| + next unless License.feature_available?(:runner_performance_insights) + + if build.failure_reason == 'runner_system_failure' && build.runner.instance_type? + ::Ci::RunnerFailedBuild.upsert_from_build!(build) + end + end end end diff --git a/ee/spec/models/ci/build_spec.rb b/ee/spec/models/ci/build_spec.rb index ae68d2175a6639..453c3e602b10ed 100644 --- a/ee/spec/models/ci/build_spec.rb +++ b/ee/spec/models/ci/build_spec.rb @@ -106,6 +106,86 @@ end end + describe 'status' do + context 'when transitioning to any state from running' do + using RSpec::Parameterized::TableSyntax + + let(:build) { create(:ci_build, :running, :with_runner_session, pipeline: pipeline) } + + where(:event) { %w(success drop cancel) } + + subject(:fire_event) { build.fire_events!(event) } + + with_them do + it 'does not create RunnerFailedBuild' do + fire_event + + expect(::Ci::RunnerFailedBuild.count).to eq 0 + end + end + end + + context 'when transitioning from running state to failed', :freeze_time do + let_it_be(:runner) { create(:ci_runner) } + + let(:build) { create(:ci_build, :running, pipeline: pipeline, runner: runner) } + + subject(:failure) { build.drop(failure_reason) } + + context 'with failure_reason not set to :runner_system_failure' do + using RSpec::Parameterized::TableSyntax + + where(:failure_reason) { Enums::Ci::CommitStatus.failure_reasons.keys - [:runner_system_failure] } + + with_them do + context 'with runner_performance_insights licensed feature' do + before do + stub_licensed_features(runner_performance_insights: true) + end + + it 'does not create RunnerFailedBuild' do + expect { failure }.not_to change { Ci::RunnerFailedBuild.count }.from(0) + end + end + end + end + + context 'with failure_reason set to :runner_system_failure' do + let(:failure_reason) { :runner_system_failure } + + context 'without runner_performance_insights licensed feature' do + before do + stub_licensed_features(runner_performance_insights: false) + end + + it 'does not create RunnerFailedBuild' do + expect { failure }.not_to change { Ci::RunnerFailedBuild.count }.from(0) + end + end + + context 'with runner_performance_insights licensed feature' do + before do + stub_licensed_features(runner_performance_insights: true) + end + + it 'creates RunnerFailedBuild' do + expect { failure }.to change { Ci::RunnerFailedBuild.count }.from(0).to(1) + + expect(::Ci::RunnerFailedBuild.last).to have_attributes(build: build, finished_at: build.finished_at) + end + + context 'when runner is not an instance runner' do + let_it_be(:runner) { create(:ci_runner, :project) } + + it 'does not create RunnerFailedBuild' do + expect { failure }.not_to change { ::Ci::RunnerFailedBuild.count }.from(0) + end + end + end + end + end + end + describe '#variables' do subject { job.variables } diff --git a/ee/spec/models/ci/runner_failed_build_spec.rb b/ee/spec/models/ci/runner_failed_build_spec.rb index 225facba784f64..0035cf1110bd10 100644 --- a/ee/spec/models/ci/runner_failed_build_spec.rb +++ b/ee/spec/models/ci/runner_failed_build_spec.rb @@ -51,4 +51,16 @@ it { expect(partitioning_strategy.current_partitions).to include partitioning_strategy.initial_partition } it { expect(partitioning_strategy.active_partition).to be_present } end + + describe '.upsert_from_build!' do + subject(:upsert_from_build) { described_class.upsert_from_build!(build) } + + let_it_be(:build) { create(:ci_build, :failed, failure_reason: :runner_system_failure) } + + it 'inserts new record' do + expect { upsert_from_build } + .to change { described_class.where(build_id: build.id).count }.from(0).to(1) + .and change { Ci::RunnerFailedBuild.count }.by(1) + end + end end -- GitLab From 133c6099f4e203ceab6ee671030861bacf16245c Mon Sep 17 00:00:00 2001 From: Pedro Pombeiro Date: Thu, 24 Aug 2023 14:22:00 +0200 Subject: [PATCH 2/2] Add runner_failed_builds FF --- .../development/runner_failed_builds.yml | 8 ++++++++ ee/app/models/ci/runner_failed_build.rb | 2 ++ ee/spec/models/ci/build_spec.rb | 10 ++++++++++ ee/spec/models/ci/runner_failed_build_spec.rb | 11 +++++++++++ 4 files changed, 31 insertions(+) create mode 100644 config/feature_flags/development/runner_failed_builds.yml diff --git a/config/feature_flags/development/runner_failed_builds.yml b/config/feature_flags/development/runner_failed_builds.yml new file mode 100644 index 00000000000000..4dead9111128a1 --- /dev/null +++ b/config/feature_flags/development/runner_failed_builds.yml @@ -0,0 +1,8 @@ +--- +name: runner_failed_builds +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/130174 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/423265 +milestone: '16.4' +type: development +group: group::runner +default_enabled: false diff --git a/ee/app/models/ci/runner_failed_build.rb b/ee/app/models/ci/runner_failed_build.rb index b3489faba55b98..85369419e0581b 100644 --- a/ee/app/models/ci/runner_failed_build.rb +++ b/ee/app/models/ci/runner_failed_build.rb @@ -16,6 +16,8 @@ class RunnerFailedBuild < Ci::ApplicationRecord class << self def upsert_from_build!(build) + return if Feature.disabled?(:runner_failed_builds) + entry = new(args_from_build(build)) entry.validate! diff --git a/ee/spec/models/ci/build_spec.rb b/ee/spec/models/ci/build_spec.rb index 453c3e602b10ed..a81a7c8bd45156 100644 --- a/ee/spec/models/ci/build_spec.rb +++ b/ee/spec/models/ci/build_spec.rb @@ -181,6 +181,16 @@ expect { failure }.not_to change { ::Ci::RunnerFailedBuild.count }.from(0) end end + + context 'when runner_failed_builds FF is disabled' do + before do + stub_feature_flags(runner_failed_builds: false) + end + + it 'does not create RunnerFailedBuild' do + expect { failure }.not_to change { ::Ci::RunnerFailedBuild.count }.from(0) + end + end end end end diff --git a/ee/spec/models/ci/runner_failed_build_spec.rb b/ee/spec/models/ci/runner_failed_build_spec.rb index 0035cf1110bd10..2c45853f9438ed 100644 --- a/ee/spec/models/ci/runner_failed_build_spec.rb +++ b/ee/spec/models/ci/runner_failed_build_spec.rb @@ -62,5 +62,16 @@ .to change { described_class.where(build_id: build.id).count }.from(0).to(1) .and change { Ci::RunnerFailedBuild.count }.by(1) end + + context 'when runner_failed_builds FF is disabled' do + before do + stub_feature_flags(runner_failed_builds: false) + end + + it 'does not insert new record' do + expect { upsert_from_build }.not_to change { described_class.where(build_id: build.id).count }.from(0) + expect(upsert_from_build).to be_nil + end + end end end -- GitLab