Merge branch 'update-runner-information' into 'master'

Update runner version only when updating contacted_at

## What does this MR do?

Improves how we update runners table, especially the version.

This is another round of improvements to reduce number of `ci_runners` updates.

I did make `contacted_at` to be updated more often (on average every 15 minutes).
We will also update version information in one go to solve: https://gitlab.com/gitlab-org/gitlab-ce/issues/22206

Improves: https://gitlab.com/gitlab-org/gitlab-ce/issues/22590
Solves: https://gitlab.com/gitlab-org/gitlab-ce/issues/22206

See merge request !6537

5 files changed, 48 insertions, 33 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 51515ac5ade..49b15c1773b 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -1,6 +1,7 @@
 Please view this file on the master branch, on stable branches it's out of date.
 
 v 8.13.0 (unreleased)
+  - Update runner version only when updating contacted_at
   - Add link from system note to compare with previous version
   - Use gitlab-shell v3.6.2 (GIT TRACE logging)
   - Fix centering of custom header logos (Ashley Dumaine)
diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb
index ed5d4b13b7e..44cb19ece3b 100644
--- a/app/models/ci/runner.rb
+++ b/app/models/ci/runner.rb
@@ -2,7 +2,7 @@ module Ci
   class Runner < ActiveRecord::Base
     extend Ci::Model
 
-    LAST_CONTACT_TIME = 2.hours.ago
+    LAST_CONTACT_TIME = 1.hour.ago
     AVAILABLE_SCOPES = %w[specific shared active paused online]
     FORM_EDITABLE = %i[description tag_list active run_untagged locked]
 
diff --git a/lib/ci/api/builds.rb b/lib/ci/api/builds.rb
index 59f85416ee5..ed87a2603e8 100644
--- a/lib/ci/api/builds.rb
+++ b/lib/ci/api/builds.rb
@@ -12,10 +12,9 @@ module Ci
         #   POST /builds/register
         post "register" do
           authenticate_runner!
-          update_runner_last_contact(save: false)
-          update_runner_info
           required_attributes! [:token]
           not_found! unless current_runner.active?
+          update_runner_info
 
           build = Ci::RegisterBuildService.new.execute(current_runner)
 
@@ -41,10 +40,11 @@ module Ci
         #   PUT /builds/:id
         put ":id" do
           authenticate_runner!
-          update_runner_last_contact
           build = Ci::Build.where(runner_id: current_runner.id).running.find(params[:id])
           forbidden!('Build has been erased!') if build.erased?
 
+          update_runner_info
+
           build.update_attributes(trace: params[:trace]) if params[:trace]
 
           Gitlab::Metrics.add_event(:update_build,
diff --git a/lib/ci/api/helpers.rb b/lib/ci/api/helpers.rb
index 23353c62885..e608f5f6cad 100644
--- a/lib/ci/api/helpers.rb
+++ b/lib/ci/api/helpers.rb
@@ -3,7 +3,7 @@ module Ci
     module Helpers
       BUILD_TOKEN_HEADER = "HTTP_BUILD_TOKEN"
       BUILD_TOKEN_PARAM = :token
-      UPDATE_RUNNER_EVERY = 40 * 60
+      UPDATE_RUNNER_EVERY = 10 * 60
 
       def authenticate_runners!
         forbidden! unless runner_registration_token_valid?
@@ -30,14 +30,22 @@ module Ci
         token && (build.valid_token?(token) || build.project.valid_runners_token?(token))
       end
 
-      def update_runner_last_contact(save: true)
-        # Use a random threshold to prevent beating DB updates
-        # it generates a distribution between: [40m, 80m]
+      def update_runner_info
+        return unless update_runner?
+
+        current_runner.contacted_at = Time.now
+        current_runner.assign_attributes(get_runner_version_from_params)
+        current_runner.save if current_runner.changed?
+      end
+
+      def update_runner?
+        # Use a random threshold to prevent beating DB updates.
+        # It generates a distribution between [40m, 80m].
+        #
         contacted_at_max_age = UPDATE_RUNNER_EVERY + Random.rand(UPDATE_RUNNER_EVERY)
-        if current_runner.contacted_at.nil? || Time.now - current_runner.contacted_at >= contacted_at_max_age
-          current_runner.contacted_at = Time.now
-          current_runner.save if current_runner.changed? && save
-        end
+
+        current_runner.contacted_at.nil? ||
+          (Time.now - current_runner.contacted_at) >= contacted_at_max_age
       end
 
       def build_not_found!
@@ -57,11 +65,6 @@ module Ci
         attributes_for_keys(["name", "version", "revision", "platform", "architecture"], params["info"])
       end
 
-      def update_runner_info
-        current_runner.assign_attributes(get_runner_version_from_params)
-        current_runner.save if current_runner.changed?
-      end
-
       def max_artifacts_size
         current_application_settings.max_artifacts_size.megabytes.to_i
       end
diff --git a/spec/requests/ci/api/builds_spec.rb b/spec/requests/ci/api/builds_spec.rb
index df97f1bf7b6..7b7d62feb2c 100644
--- a/spec/requests/ci/api/builds_spec.rb
+++ b/spec/requests/ci/api/builds_spec.rb
@@ -35,18 +35,24 @@ describe Ci::API::API do
         end
       end
 
-      it "starts a build" do
-        register_builds info: { platform: :darwin }
-
-        expect(response).to have_http_status(201)
-        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"] })
-        expect(json_response["variables"]).to include(
-          { "key" => "CI_BUILD_NAME", "value" => "spinach", "public" => true },
-          { "key" => "CI_BUILD_STAGE", "value" => "test", "public" => true },
-          { "key" => "DB_NAME", "value" => "postgres", "public" => true }
-        )
+      context 'when there is a pending build' do
+        it 'starts a build' do
+          register_builds info: { platform: :darwin }
+
+          expect(response).to have_http_status(201)
+          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"] })
+          expect(json_response["variables"]).to include(
+            { "key" => "CI_BUILD_NAME", "value" => "spinach", "public" => true },
+            { "key" => "CI_BUILD_STAGE", "value" => "test", "public" => true },
+            { "key" => "DB_NAME", "value" => "postgres", "public" => true }
+          )
+        end
+
+        it 'updates runner info' do
+          expect { register_builds }.to change { runner.reload.contacted_at }
+        end
       end
 
       context 'when builds are finished' do
@@ -159,13 +165,18 @@ describe Ci::API::API do
       end
 
       context 'when runner is paused' do
-        let(:inactive_runner) { create(:ci_runner, :inactive, token: "InactiveRunner") }
+        let(:runner) { create(:ci_runner, :inactive, token: 'InactiveRunner') }
 
-        before do
-          register_builds inactive_runner.token
+        it 'responds with 404' do
+          register_builds
+
+          expect(response).to have_http_status 404
         end
 
-        it { expect(response).to have_http_status 404 }
+        it 'does not update runner info' do
+          expect { register_builds }
+            .not_to change { runner.reload.contacted_at }
+        end
       end
 
       def register_builds(token = runner.token, **params)