Merge branch '21698-redis-runner-last-build' into 'master'

Reduce DB-load for build-queues by storing last_update in Redis

See merge request !8084

11 files changed, 175 insertions, 7 deletions

diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index 48ffe40abc6..f6f93debaf3 100644
--- a/app/models/ci/build.rb
+++ b/app/models/ci/build.rb
@@ -91,6 +91,12 @@ module Ci
     end
 
     state_machine :status do
+      after_transition any => [:pending] do |build|
+        build.run_after_commit do
+          BuildQueueWorker.perform_async(id)
+        end
+      end
+
       after_transition pending: :running do |build|
         build.run_after_commit do
           BuildHooksWorker.perform_async(id)
diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb
index 123930273e0..6e58a1878c8 100644
--- a/app/models/ci/runner.rb
+++ b/app/models/ci/runner.rb
@@ -2,6 +2,7 @@ module Ci
   class Runner < ActiveRecord::Base
     extend Ci::Model
 
+    RUNNER_QUEUE_EXPIRY_TIME = 60.minutes
     LAST_CONTACT_TIME = 1.hour.ago
     AVAILABLE_SCOPES = %w[specific shared active paused online]
     FORM_EDITABLE = %i[description tag_list active run_untagged locked]
@@ -21,6 +22,8 @@ module Ci
     scope :online, ->() { where('contacted_at > ?', LAST_CONTACT_TIME) }
     scope :ordered, ->() { order(id: :desc) }
 
+    after_save :tick_runner_queue, if: :form_editable_changed?
+
     scope :owned_or_shared, ->(project_id) do
       joins('LEFT JOIN ci_runner_projects ON ci_runner_projects.runner_id = ci_runners.id')
         .where("ci_runner_projects.gl_project_id = :project_id OR ci_runners.is_shared = true", project_id: project_id)
@@ -122,8 +125,36 @@ module Ci
       ]
     end
 
+    def tick_runner_queue
+      new_update = SecureRandom.hex
+      Gitlab::Redis.with { |redis| redis.set(runner_queue_key, new_update, ex: RUNNER_QUEUE_EXPIRY_TIME) }
+      new_update
+    end
+
+    def ensure_runner_queue_value
+      Gitlab::Redis.with do |redis|
+        value = SecureRandom.hex
+        redis.set(runner_queue_key, value, ex: RUNNER_QUEUE_EXPIRY_TIME, nx: true)
+        redis.get(runner_queue_key)
+      end
+    end
+
+    def is_runner_queue_value_latest?(value)
+      ensure_runner_queue_value == value if value.present?
+    end
+
     private
 
+    def runner_queue_key
+      "runner:build_queue:#{self.token}"
+    end
+
+    def form_editable_changed?
+      FORM_EDITABLE.any? do |editable|
+        public_send("#{editable}_changed?")
+      end
+    end
+
     def tag_constraints
       unless has_tags? || run_untagged?
         errors.add(:tags_list,
diff --git a/app/services/ci/update_build_queue_service.rb b/app/services/ci/update_build_queue_service.rb
new file mode 100644
index 00000000000..2e901016666
--- /dev/null
+++ b/app/services/ci/update_build_queue_service.rb
@@ -0,0 +1,11 @@
+module Ci
+  class UpdateBuildQueueService
+    def execute(build)
+      build.project.runners.each do |runner|
+        if runner.can_pick?(build)
+          runner.tick_runner_queue
+        end
+      end
+    end
+  end
+end
diff --git a/app/workers/build_queue_worker.rb b/app/workers/build_queue_worker.rb
new file mode 100644
index 00000000000..fa9e097e40a
--- /dev/null
+++ b/app/workers/build_queue_worker.rb
@@ -0,0 +1,10 @@
+class BuildQueueWorker
+  include Sidekiq::Worker
+  include BuildQueue
+
+  def perform(build_id)
+    Ci::Build.find_by(id: build_id).try do |build|
+      Ci::UpdateBuildQueueService.new.execute(build)
+    end
+  end
+end
diff --git a/changelogs/unreleased/21698-redis-runner-last-build.yml b/changelogs/unreleased/21698-redis-runner-last-build.yml
new file mode 100644
index 00000000000..2aba3c353a3
--- /dev/null
+++ b/changelogs/unreleased/21698-redis-runner-last-build.yml
@@ -0,0 +1,4 @@
+---
+title: Reduce DB-load for build-queues by storing last_update in Redis
+merge_request: 8084
+author:
diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb
index 6b81fbf294e..49c5f0652ab 100644
--- a/lib/api/helpers.rb
+++ b/lib/api/helpers.rb
@@ -226,7 +226,7 @@ module API
     end
 
     def render_api_error!(message, status)
-      error!({ 'message' => message }, status)
+      error!({ 'message' => message }, status, header)
     end
 
     def handle_api_exception(exception)
diff --git a/lib/ci/api/builds.rb b/lib/ci/api/builds.rb
index 142bce82286..c4bdef781f7 100644
--- a/lib/ci/api/builds.rb
+++ b/lib/ci/api/builds.rb
@@ -16,6 +16,13 @@ module Ci
           not_found! unless current_runner.active?
           update_runner_info
 
+          if current_runner.is_runner_queue_value_latest?(params[:last_update])
+            header 'X-GitLab-Last-Update', params[:last_update]
+            return build_not_found!
+          end
+
+          new_update = current_runner.ensure_runner_queue_value
+
           build = Ci::RegisterBuildService.new.execute(current_runner)
 
           if build
@@ -26,6 +33,8 @@ module Ci
           else
             Gitlab::Metrics.add_event(:build_not_found)
 
+            header 'X-GitLab-Last-Update', new_update
+
             build_not_found!
           end
         end
diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb
index 3309a7fff9f..f031876e812 100644
--- a/spec/models/ci/build_spec.rb
+++ b/spec/models/ci/build_spec.rb
@@ -1401,4 +1401,14 @@ describe Ci::Build, :models do
       it { is_expected.to eq(%w[predefined project pipeline yaml secret]) }
     end
   end
+
+  describe 'State transition: any => [:pending]' do
+    let(:build) { create(:ci_build, :created) }
+
+    it 'queues BuildQueueWorker' do
+      expect(BuildQueueWorker).to receive(:perform_async).with(build.id)
+
+      build.enqueue
+    end
+  end
 end
diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb
index ef65eb99328..2b856ca7af7 100644
--- a/spec/models/ci/runner_spec.rb
+++ b/spec/models/ci/runner_spec.rb
@@ -263,6 +263,62 @@ describe Ci::Runner, models: true do
     end
   end
 
+  describe '#tick_runner_queue' do
+    let(:runner) { create(:ci_runner) }
+
+    it 'returns a new last_update value' do
+      expect(runner.tick_runner_queue).not_to be_empty
+    end
+  end
+
+  describe '#ensure_runner_queue_value' do
+    let(:runner) { create(:ci_runner) }
+
+    it 'sets a new last_update value when it is called the first time' do
+      last_update = runner.ensure_runner_queue_value
+
+      expect_value_in_redis.to eq(last_update)
+    end
+
+    it 'does not change if it is not expired and called again' do
+      last_update = runner.ensure_runner_queue_value
+
+      expect(runner.ensure_runner_queue_value).to eq(last_update)
+      expect_value_in_redis.to eq(last_update)
+    end
+
+    context 'updates runner queue after changing editable value' do
+      let!(:last_update) { runner.ensure_runner_queue_value }
+
+      before do
+        runner.update(description: 'new runner')
+      end
+
+      it 'sets a new last_update value' do
+        expect_value_in_redis.not_to eq(last_update)
+      end
+    end
+
+    context 'does not update runner value after save' do
+      let!(:last_update) { runner.ensure_runner_queue_value }
+
+      before do
+        runner.touch
+      end
+
+      it 'has an old last_update value' do
+        expect_value_in_redis.to eq(last_update)
+      end
+    end
+
+    def expect_value_in_redis
+      Gitlab::Redis.with do |redis|
+        runner_queue_key = runner.send(:runner_queue_key)
+        expect(redis.get(runner_queue_key))
+      end
+    end
+  end
+
   describe '.assignable_for' do
     let(:runner) { create(:ci_runner) }
     let(:project) { create(:project) }
diff --git a/spec/requests/api/helpers_spec.rb b/spec/requests/api/helpers_spec.rb
index b8ee2293a33..a89676fec93 100644
--- a/spec/requests/api/helpers_spec.rb
+++ b/spec/requests/api/helpers_spec.rb
@@ -12,6 +12,7 @@ describe API::Helpers, api: true do
   let(:params) { {} }
   let(:env) { { 'REQUEST_METHOD' => 'GET' } }
   let(:request) { Rack::Request.new(env) }
+  let(:header) { }
 
   def set_env(user_or_token, identifier)
     clear_env
@@ -46,7 +47,7 @@ describe API::Helpers, api: true do
     allow_any_instance_of(self.class).to receive(:doorkeeper_guard){ value }
   end
 
-  def error!(message, status)
+  def error!(message, status, header)
     raise Exception.new("#{status} - #{message}")
   end
 
diff --git a/spec/requests/ci/api/builds_spec.rb b/spec/requests/ci/api/builds_spec.rb
index 3b5dc98e4d5..270c23e3f19 100644
--- a/spec/requests/ci/api/builds_spec.rb
+++ b/spec/requests/ci/api/builds_spec.rb
@@ -4,7 +4,8 @@ describe Ci::API::Builds do
   include ApiHelpers
 
   let(:runner) { FactoryGirl.create(:ci_runner, tag_list: ["mysql", "ruby"]) }
-  let(:project) { FactoryGirl.create(:empty_project) }
+  let(:project) { FactoryGirl.create(:empty_project, shared_runners_enabled: false) }
+  let(:last_update) { nil }
 
   describe "Builds API for runners" do
     let(:pipeline) { create(:ci_pipeline_without_jobs, project: project, ref: 'master') }
@@ -16,6 +17,8 @@ describe Ci::API::Builds do
     describe "POST /builds/register" do
       let!(:build) { create(:ci_build, pipeline: pipeline, name: 'spinach', stage: 'test', stage_idx: 0) }
       let(:user_agent) { 'gitlab-ci-multi-runner 1.5.2 (1-5-stable; go1.6.3; linux/amd64)' }
+      let!(:last_update) { }
+      let!(:new_update) { }
 
       before do
         stub_container_registry_config(enabled: false)
@@ -24,7 +27,31 @@ describe Ci::API::Builds do
       shared_examples 'no builds available' do
         context 'when runner sends version in User-Agent' do
           context 'for stable version' do
-            it { expect(response).to have_http_status(204) }
+            it 'gives 204 and set X-GitLab-Last-Update' do
+              expect(response).to have_http_status(204)
+              expect(response.header).to have_key('X-GitLab-Last-Update')
+            end
+          end
+
+          context 'when last_update is up-to-date' do
+            let(:last_update) { runner.ensure_runner_queue_value }
+
+            it 'gives 204 and set the same X-GitLab-Last-Update' do
+              expect(response).to have_http_status(204)
+              expect(response.header['X-GitLab-Last-Update'])
+                .to eq(last_update)
+            end
+          end
+
+          context 'when last_update is outdated' do
+            let(:last_update) { runner.ensure_runner_queue_value }
+            let(:new_update) { runner.tick_runner_queue }
+
+            it 'gives 204 and set a new X-GitLab-Last-Update' do
+              expect(response).to have_http_status(204)
+              expect(response.header['X-GitLab-Last-Update'])
+                .to eq(new_update)
+            end
           end
 
           context 'for beta version' do
@@ -49,6 +76,7 @@ describe Ci::API::Builds do
           register_builds info: { platform: :darwin }
 
           expect(response).to have_http_status(201)
+          expect(response.headers).not_to have_key('X-GitLab-Last-Update')
           expect(json_response['sha']).to eq(build.sha)
           expect(runner.reload.platform).to eq("darwin")
           expect(json_response["options"]).to eq({ "image" => "ruby:2.1", "services" => ["postgres"] })
@@ -119,10 +147,10 @@ describe Ci::API::Builds do
       end
 
       context 'for shared runner' do
-        let(:shared_runner) { create(:ci_runner, token: "SharedRunner") }
+        let!(:runner) { create(:ci_runner, :shared, token: "SharedRunner") }
 
         before do
-          register_builds shared_runner.token
+          register_builds(runner.token)
         end
 
         it_behaves_like 'no builds available'
@@ -224,7 +252,9 @@ describe Ci::API::Builds do
       end
 
       def register_builds(token = runner.token, **params)
-        post ci_api("/builds/register"), params.merge(token: token), { 'User-Agent' => user_agent }
+        new_params = params.merge(token: token, last_update: last_update)
+
+        post ci_api("/builds/register"), new_params, { 'User-Agent' => user_agent }
       end
     end