diff options
author | DJ Mountney <dj@gitlab.com> | 2017-03-01 08:20:01 +0000 |
---|---|---|
committer | DJ Mountney <david@twkie.net> | 2017-03-01 00:24:41 -0800 |
commit | 806a7e1b5b247eafe2decc6ca6c01a1a1b3d9583 (patch) | |
tree | 20f19c52651c5aae8f3253b33e21fef684fd0f2d | |
parent | 512bf6483192a4dac25f7bc335fa6d8b654486db (diff) | |
download | gitlab-ce-806a7e1b5b247eafe2decc6ca6c01a1a1b3d9583.tar.gz |
Merge branch 'fix-builds-processing' into 'master'
Fix concurrent access on builds/register
See merge request !9600
-rw-r--r-- | app/services/ci/register_build_service.rb | 36 | ||||
-rw-r--r-- | spec/services/ci/register_build_service_spec.rb | 45 |
2 files changed, 69 insertions, 12 deletions
diff --git a/app/services/ci/register_build_service.rb b/app/services/ci/register_build_service.rb index 6f03bf2be13..5b52a0425de 100644 --- a/app/services/ci/register_build_service.rb +++ b/app/services/ci/register_build_service.rb @@ -20,21 +20,33 @@ module Ci builds_for_specific_runner end - build = builds.find do |build| - runner.can_pick?(build) - end + valid = true - if build - # 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. - build.runner_id = runner.id - build.run! - end + builds.find do |build| + next unless runner.can_pick?(build) + + begin + # 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. + build.runner_id = runner.id + build.run! - Result.new(build, true) + return Result.new(build, true) + rescue StateMachines::InvalidTransition, ActiveRecord::StaleObjectError + # 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 - rescue StateMachines::InvalidTransition, ActiveRecord::StaleObjectError - Result.new(build, false) + Result.new(nil, valid) end private diff --git a/spec/services/ci/register_build_service_spec.rb b/spec/services/ci/register_build_service_spec.rb index d9f774a1095..cd7dd53025c 100644 --- a/spec/services/ci/register_build_service_spec.rb +++ b/spec/services/ci/register_build_service_spec.rb @@ -170,6 +170,51 @@ module Ci 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) described_class.new(runner).execute.build end |