Commit c5e0c841 authored by Grzegorz Bizon's avatar Grzegorz Bizon

Validate queuing entry before we upsert it into database

This ensures that only valid objects can be upserted into the database.
parent 0d21fb42
...@@ -37,7 +37,7 @@ module Ci ...@@ -37,7 +37,7 @@ module Ci
has_one :deployment, as: :deployable, class_name: 'Deployment' has_one :deployment, as: :deployable, class_name: 'Deployment'
has_one :pending_state, class_name: 'Ci::BuildPendingState', inverse_of: :build has_one :pending_state, class_name: 'Ci::BuildPendingState', inverse_of: :build
has_one :queuing_entry, class_name: 'Ci::PendingBuild', foreign_key: :build_id has_one :queuing_entry, class_name: 'Ci::PendingBuild', foreign_key: :build_id, inverse_of: :build
has_many :trace_sections, class_name: 'Ci::BuildTraceSection' has_many :trace_sections, class_name: 'Ci::BuildTraceSection'
has_many :trace_chunks, class_name: 'Ci::BuildTraceChunk', foreign_key: :build_id, inverse_of: :build has_many :trace_chunks, class_name: 'Ci::BuildTraceChunk', foreign_key: :build_id, inverse_of: :build
has_many :report_results, class_name: 'Ci::BuildReportResult', inverse_of: :build has_many :report_results, class_name: 'Ci::BuildReportResult', inverse_of: :build
...@@ -1038,6 +1038,14 @@ module Ci ...@@ -1038,6 +1038,14 @@ module Ci
options.dig(:allow_failure_criteria, :exit_codes).present? options.dig(:allow_failure_criteria, :exit_codes).present?
end end
def all_queuing_entries
# We can have only one queuing entry, because there is a unique index on
# `build_id`, but we need a relation to remove this single queuing entry
# more efficiently in a single statement without actually load data.
::Ci::PendingBuild.where(build_id: self.id)
end
protected protected
def run_status_commit_hooks! def run_status_commit_hooks!
......
...@@ -5,6 +5,14 @@ module Ci ...@@ -5,6 +5,14 @@ module Ci
extend Gitlab::Ci::Model extend Gitlab::Ci::Model
belongs_to :project belongs_to :project
belongs_to :build, class_name: 'Ci::Build' belongs_to :build, class_name: 'Ci::Build', inverse_of: :queuing_entry
def self.upsert!(build)
entry = self.new(build: build, project: build.project)
raise ArgumentError, 'queuing entry invalid' unless entry.valid?
upsert({ build_id: entry.build_id, project_id: entry.project_id })
end
end end
end end
...@@ -19,11 +19,9 @@ module Ci ...@@ -19,11 +19,9 @@ module Ci
raise InvalidQueueTransition unless transition.to == 'pending' raise InvalidQueueTransition unless transition.to == 'pending'
transition.within_transaction do transition.within_transaction do
attributes = { build_id: build.id, project_id: build.project.id } result = ::Ci::PendingBuild.upsert!(build)
::Ci::PendingBuild.upsert(attributes).then do |result|
raise ArgumentError if result.length > 1
unless result.empty?
metrics.increment_queue_operation(:build_queue_push) metrics.increment_queue_operation(:build_queue_push)
result.rows.dig(0, 0) result.rows.dig(0, 0)
...@@ -40,12 +38,12 @@ module Ci ...@@ -40,12 +38,12 @@ module Ci
raise InvalidQueueTransition unless transition.from == 'pending' raise InvalidQueueTransition unless transition.from == 'pending'
transition.within_transaction do transition.within_transaction do
::Ci::PendingBuild.where(build_id: build.id).delete_all.then do |removed| # rubocop:disable CodeReuse/ActiveRecord removed = build.all_queuing_entries.delete_all
if removed > 0 if removed > 0
metrics.increment_queue_operation(:build_queue_pop) metrics.increment_queue_operation(:build_queue_pop)
end
removed > 0 build.id
end end
end end
end end
......
...@@ -44,7 +44,7 @@ RSpec.describe Ci::UpdateBuildQueueService do ...@@ -44,7 +44,7 @@ RSpec.describe Ci::UpdateBuildQueueService do
context 'when duplicate entry exists' do context 'when duplicate entry exists' do
before do before do
::Ci::PendingBuild.create(build: build, project: project) ::Ci::PendingBuild.create!(build: build, project: project)
end end
it 'does nothing and returns updated build id' do it 'does nothing and returns updated build id' do
...@@ -71,7 +71,7 @@ RSpec.describe Ci::UpdateBuildQueueService do ...@@ -71,7 +71,7 @@ RSpec.describe Ci::UpdateBuildQueueService do
it 'removes pending build in a transaction' do it 'removes pending build in a transaction' do
dequeued = subject.pop(build, transition) dequeued = subject.pop(build, transition)
expect(dequeued).to be true expect(dequeued).to eq build.id
end end
it 'increments queue pop metric' do it 'increments queue pop metric' do
...@@ -89,7 +89,7 @@ RSpec.describe Ci::UpdateBuildQueueService do ...@@ -89,7 +89,7 @@ RSpec.describe Ci::UpdateBuildQueueService do
it 'does nothing if there is no pending build to remove' do it 'does nothing if there is no pending build to remove' do
dequeued = subject.pop(build, transition) dequeued = subject.pop(build, transition)
expect(dequeued).to be false expect(dequeued).to be_nil
end end
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment