From 08e31875c2d345c0755707ab73c1fd71b1fd9b05 Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Tue, 27 Sep 2016 11:46:13 +0200 Subject: [PATCH 1/3] Update runner version only when updating contacted_at --- CHANGELOG | 1 + app/models/ci/runner.rb | 2 +- lib/ci/api/builds.rb | 6 +++--- lib/ci/api/helpers.rb | 18 +++++++----------- 4 files changed, 12 insertions(+), 15 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index a52ac53bae7..e5902587bcc 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..c6a83682e32 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,15 @@ module Ci token && (build.valid_token?(token) || build.project.valid_runners_token?(token)) end - def update_runner_last_contact(save: true) + def update_runner_info # 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 + return unless current_runner.contacted_at.nil? || Time.now - current_runner.contacted_at >= contacted_at_max_age + + current_runner.contacted_at = Time.now + current_runner.assign_attributes(get_runner_version_from_params) + current_runner.save if current_runner.changed? end def build_not_found! @@ -57,11 +58,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 From 9a8e486307550aa1a590c769d9e71621c5ce8c62 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Mon, 3 Oct 2016 10:47:37 +0200 Subject: [PATCH 2/3] Extract method that checks if ci runner needs update --- lib/ci/api/helpers.rb | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/lib/ci/api/helpers.rb b/lib/ci/api/helpers.rb index c6a83682e32..e608f5f6cad 100644 --- a/lib/ci/api/helpers.rb +++ b/lib/ci/api/helpers.rb @@ -31,16 +31,23 @@ module Ci end def update_runner_info - # 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) - return unless current_runner.contacted_at.nil? || Time.now - current_runner.contacted_at >= contacted_at_max_age + 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) + + current_runner.contacted_at.nil? || + (Time.now - current_runner.contacted_at) >= contacted_at_max_age + end + def build_not_found! if headers['User-Agent'].match(/gitlab-ci-multi-runner \d+\.\d+\.\d+(~beta\.\d+\.g[0-9a-f]+)? /) no_content! From aa1ab8aa1786517544d0a5c1a4217f98d37ea58f Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Mon, 3 Oct 2016 11:49:17 +0200 Subject: [PATCH 3/3] Add some tests for updating ci runner information --- spec/requests/ci/api/builds_spec.rb | 41 ++++++++++++++++++----------- 1 file changed, 26 insertions(+), 15 deletions(-) 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 } + 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 } - ) + 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)