Commit 806a7e1b authored by DJ Mountney's avatar DJ Mountney Committed by DJ Mountney

Merge branch 'fix-builds-processing' into 'master'

Fix concurrent access on builds/register

See merge request !9600
parent 512bf648
...@@ -20,21 +20,33 @@ module Ci ...@@ -20,21 +20,33 @@ module Ci
builds_for_specific_runner builds_for_specific_runner
end end
build = builds.find do |build| valid = true
runner.can_pick?(build)
end builds.find do |build|
next unless runner.can_pick?(build)
if build begin
# In case when 2 runners try to assign the same build, second runner will be declined # In case when 2 runners try to assign the same build, second runner will be declined
# with StateMachines::InvalidTransition or StaleObjectError when doing run! or save method. # with StateMachines::InvalidTransition or StaleObjectError when doing run! or save method.
build.runner_id = runner.id build.runner_id = runner.id
build.run! build.run!
end
Result.new(build, true)
return Result.new(build, true)
rescue StateMachines::InvalidTransition, ActiveRecord::StaleObjectError rescue StateMachines::InvalidTransition, ActiveRecord::StaleObjectError
Result.new(build, false) # We are looping to find another build that is not conflicting
# It also indicates that this build can be picked and passed to runner.
# If we don't do it, basically a bunch of runners would be competing for a build
# and thus we will generate a lot of 409. This will increase
# the number of generated requests, also will reduce significantly
# how many builds can be picked by runner in a unit of time.
# In case we hit the concurrency-access lock,
# we still have to return 409 in the end,
# to make sure that this is properly handled by runner.
valid = false
end
end
Result.new(nil, valid)
end end
private private
......
...@@ -170,6 +170,51 @@ module Ci ...@@ -170,6 +170,51 @@ module Ci
end end
end end
context 'when first build is stalled' do
before do
pending_build.lock_version = 10
end
subject { described_class.new(specific_runner).execute }
context 'with multiple builds are in queue' do
let!(:other_build) { create :ci_build, pipeline: pipeline }
before do
allow_any_instance_of(Ci::RegisterBuildService).to receive(:builds_for_specific_runner)
.and_return([pending_build, other_build])
end
it "receives second build from the queue" do
expect(subject).to be_valid
expect(subject.build).to eq(other_build)
end
end
context 'when single build is in queue' do
before do
allow_any_instance_of(Ci::RegisterBuildService).to receive(:builds_for_specific_runner)
.and_return([pending_build])
end
it "does not receive any valid result" do
expect(subject).not_to be_valid
end
end
context 'when there is no build in queue' do
before do
allow_any_instance_of(Ci::RegisterBuildService).to receive(:builds_for_specific_runner)
.and_return([])
end
it "does not receive builds but result is valid" do
expect(subject).to be_valid
expect(subject.build).to be_nil
end
end
end
def execute(runner) def execute(runner)
described_class.new(runner).execute.build described_class.new(runner).execute.build
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