Move info update implementation to Ci::Runner model

3 files changed, 71 insertions, 34 deletions

diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb
index 7e616ee9144..44be247bcd3 100644
--- a/app/models/ci/runner.rb
+++ b/app/models/ci/runner.rb
@@ -5,6 +5,7 @@ module Ci
 
     RUNNER_QUEUE_EXPIRY_TIME = 60.minutes
     ONLINE_CONTACT_TIMEOUT = 1.hour
+    UPDATE_DB_RUNNER_INFO_EVERY = 1.hour
     AVAILABLE_SCOPES = %w[specific shared active paused online].freeze
     FORM_EDITABLE = %i[description tag_list active run_untagged locked access_level].freeze
 
@@ -89,7 +90,7 @@ module Ci
 
     def online?
       Gitlab::Redis::SharedState.with do |redis|
-        last_seen = redis.get("#{self.runner_info_key}:contacted_at") || contacted_at
+        last_seen = redis.get("#{runner_info_redis_cache_key}:contacted_at") || contacted_at
         last_seen && last_seen > self.class.contact_time_deadline
       end
     end
@@ -155,8 +156,16 @@ module Ci
       ensure_runner_queue_value == value if value.present?
     end
 
-    def runner_info_key
-      "runner:info:#{self.id}"
+    def update_runner_info(params)
+      update_runner_info_cache
+
+      # Use a 1h threshold to prevent beating DB updates.
+      return unless self.contacted_at.nil? ||
+          (Time.now - self.contacted_at) >= UPDATE_DB_RUNNER_INFO_EVERY
+
+      self.contacted_at = Time.now
+      self.assign_attributes(params)
+      self.save if self.changed?
     end
 
     private
@@ -171,6 +180,17 @@ module Ci
       "runner:build_queue:#{self.token}"
     end
 
+    def runner_info_redis_cache_key
+      "runner:info:#{self.id}"
+    end
+
+    def update_runner_info_cache
+      Gitlab::Redis::SharedState.with do |redis|
+        redis_key = "#{runner_info_redis_cache_key}:contacted_at"
+        redis.set(redis_key, Time.now)
+      end
+    end
+
     def tag_constraints
       unless has_tags? || run_untagged?
         errors.add(:tags_list,
diff --git a/lib/api/helpers/runner.rb b/lib/api/helpers/runner.rb
index 5ac9181f878..8f45cae0e60 100644
--- a/lib/api/helpers/runner.rb
+++ b/lib/api/helpers/runner.rb
@@ -5,7 +5,6 @@ module API
 
       JOB_TOKEN_HEADER = 'HTTP_JOB_TOKEN'.freeze
       JOB_TOKEN_PARAM = :token
-      UPDATE_RUNNER_EVERY = 10 * 60
 
       def runner_registration_token_valid?
         ActiveSupport::SecurityUtils.variable_size_secure_compare(params[:token],
@@ -21,37 +20,13 @@ module API
       def authenticate_runner!
         forbidden! unless current_runner
 
-        update_runner_info
+        current_runner.update_runner_info(get_runner_version_from_params)
       end
 
       def current_runner
         @runner ||= ::Ci::Runner.find_by_token(params[:token].to_s)
       end
 
-      def update_runner_info
-        update_runner_info_cache
-
-        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 1h threshold to prevent beating DB updates.
-
-        current_runner.contacted_at.nil? ||
-          (Time.now - current_runner.contacted_at) >= UPDATE_RUNNER_EVERY
-      end
-
-      def update_runner_info_cache
-        Gitlab::Redis::SharedState.with do |redis|
-          redis_key = "#{current_runner.runner_info_key}:contacted_at"
-          redis.set(redis_key, Time.now)
-        end
-      end
-
       def validate_job!(job)
         not_found! unless job
 
diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb
index fe9e5ec9436..830f7cd68c5 100644
--- a/spec/models/ci/runner_spec.rb
+++ b/spec/models/ci/runner_spec.rb
@@ -97,7 +97,7 @@ describe Ci::Runner do
 
     context 'no cache value' do
       before do
-        stub_redis("#{runner.runner_info_key}:contacted_at", nil)
+        stub_redis_runner_contacted_at(nil)
       end
 
       context 'never contacted' do
@@ -128,7 +128,7 @@ describe Ci::Runner do
     context 'with cache value' do
       context 'contacted long time ago time' do
         before do
-          stub_redis("#{runner.runner_info_key}:contacted_at", 1.year.ago.to_s)
+          stub_redis_runner_contacted_at(1.year.ago.to_s)
         end
 
         it { is_expected.to be_falsey }
@@ -136,17 +136,17 @@ describe Ci::Runner do
 
       context 'contacted 1s ago' do
         before do
-          stub_redis("#{runner.runner_info_key}:contacted_at", 1.second.ago.to_s)
+          stub_redis_runner_contacted_at(1.second.ago.to_s)
         end
 
         it { is_expected.to be_truthy }
       end
     end
 
-    def stub_redis(key, value)
+    def stub_redis_runner_contacted_at(value)
       redis = double
       allow(Gitlab::Redis::SharedState).to receive(:with).and_yield(redis)
-      allow(redis).to receive(:get).with(key).and_return(value)
+      allow(redis).to receive(:get).with("#{runner.send(:runner_info_redis_cache_key)}:contacted_at").and_return(value)
     end
   end
 
@@ -391,6 +391,48 @@ describe Ci::Runner do
     end
   end
 
+  describe '#update_runner_info' do
+    let(:runner) { create(:ci_runner) }
+
+    subject { runner.update_runner_info(contacted_at: Time.now) }
+
+    context 'when database was updated recently' do
+      before do
+        runner.update(contacted_at: Time.now)
+      end
+
+      it 'updates cache' do
+        expect_redis_update
+
+        subject
+      end
+    end
+
+    context 'when database was not updated recently' do
+      before do
+        runner.update(contacted_at: 2.hours.ago)
+      end
+
+      it 'updates database' do
+        expect_redis_update
+
+        expect { subject }.to change { runner.reload.contacted_at }
+      end
+
+      it 'updates cache' do
+        expect_redis_update
+
+        subject
+      end
+    end
+
+    def expect_redis_update
+      redis = double
+      expect(Gitlab::Redis::SharedState).to receive(:with).and_yield(redis)
+      expect(redis).to receive(:set).with("#{runner.send(:runner_info_redis_cache_key)}:contacted_at", anything)
+    end
+  end
+
   describe '#destroy' do
     let(:runner) { create(:ci_runner) }