Improve runner attribute cachine

3 files changed, 38 insertions, 41 deletions

diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb
index 5711be3ae29..6bd90e71bf3 100644
--- a/app/models/ci/runner.rb
+++ b/app/models/ci/runner.rb
@@ -2,12 +2,14 @@ module Ci
   class Runner < ActiveRecord::Base
     extend Gitlab::Ci::Model
     include Gitlab::SQL::Pattern
+    include Gitlab::Utils::StrongMemoize
 
     RUNNER_QUEUE_EXPIRY_TIME = 60.minutes
     ONLINE_CONTACT_TIMEOUT = 1.hour
     UPDATE_DB_RUNNER_INFO_EVERY = 40.minutes
     AVAILABLE_SCOPES = %w[specific shared active paused online].freeze
     FORM_EDITABLE = %i[description tag_list active run_untagged locked access_level].freeze
+    CACHED_ATTRIBUTES_EXPIRY_TIME = 24.hours
 
     has_many :builds
     has_many :runner_projects, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
@@ -68,29 +70,15 @@ module Ci
       ONLINE_CONTACT_TIMEOUT.ago
     end
 
-    def name
-      cached_attribute(:name) || read_attribute(:name)
-    end
-
-    def version
-      cached_attribute(:version) || read_attribute(:version)
-    end
-
-    def revision
-      cached_attribute(:revision) || read_attribute(:revision)
-    end
-
-    def platform
-      cached_attribute(:platform) || read_attribute(:platform)
-    end
-
-    def architecture
-      cached_attribute(:architecture) || read_attribute(:architecture)
+    def self.cached_attr_reader(*attributes)
+      attributes.each do |attribute|
+        define_method("#{attribute}") do
+          cached_attribute(attribute) || read_attribute(attribute)
+        end
+      end
     end
 
-    def contacted_at
-      cached_attribute(:contacted_at) || read_attribute(:contacted_at)
-    end
+    cached_attr_reader :version, :revision, :platform, :architecture, :contacted_at
 
     def set_default_values
       self.token = SecureRandom.hex(15) if self.token.blank?
@@ -178,7 +166,7 @@ module Ci
     end
 
     def update_cached_info(values)
-      values = values&.slice(:name, :version, :revision, :platform, :architecture) || {}
+      values = values&.slice(:version, :revision, :platform, :architecture) || {}
       values[:contacted_at] = Time.now
 
       cache_attributes(values)
@@ -201,8 +189,8 @@ module Ci
       "runner:build_queue:#{self.token}"
     end
 
-    def cache_attribute_key(key)
-      "runner:info:#{self.id}:#{key}"
+    def cache_attribute_key
+      "runner:info:#{self.id}"
     end
 
     def persist_cached_data?
@@ -217,17 +205,21 @@ module Ci
     end
 
     def cached_attribute(key)
-      @cached_attributes = {}
-      @cached_attributes[key] ||= Gitlab::Redis::SharedState.with do |redis|
-        redis.get(cache_attribute_key(key))
+      (cached_attributes || {})[key]
+    end
+
+    def cached_attributes
+      strong_memoize(:cached_attributes) do
+        Gitlab::Redis::SharedState.with do |redis|
+          data = redis.get(cache_attribute_key)
+          JSON.parse(data, symbolize_names: true) if data
+        end
       end
     end
 
     def cache_attributes(values)
       Gitlab::Redis::SharedState.with do |redis|
-        values.each do |key, value|
-          redis.set(cache_attribute_key(key), value, ex: 24.hours)
-        end
+        redis.set(cache_attribute_key, values.to_json, ex: CACHED_ATTRIBUTES_EXPIRY_TIME)
       end
     end
 
diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb
index 41e27c62bd9..ab170e6351c 100644
--- a/spec/models/ci/runner_spec.rb
+++ b/spec/models/ci/runner_spec.rb
@@ -95,6 +95,12 @@ describe Ci::Runner do
 
     subject { runner.online? }
 
+    before do
+      allow_any_instance_of(described_class).to receive(:cached_attribute).and_call_original
+      allow_any_instance_of(described_class).to receive(:cached_attribute)
+        .with(:platform).and_return("darwin")
+    end
+
     context 'no cache value' do
       before do
         stub_redis_runner_contacted_at(nil)
@@ -147,8 +153,9 @@ describe Ci::Runner do
 
     def stub_redis_runner_contacted_at(value)
       Gitlab::Redis::SharedState.with do |redis|
-        cache_key = runner.send(:cache_attribute_key, :contacted_at)
-        expect(redis).to receive(:get).with(cache_key).and_return(value).at_least(:once)
+        cache_key = runner.send(:cache_attribute_key)
+        expect(redis).to receive(:get).with(cache_key)
+          .and_return({ contacted_at: value }.to_json).at_least(:once)
       end
     end
   end
@@ -405,7 +412,7 @@ describe Ci::Runner do
       end
 
       it 'updates cache' do
-        expect_redis_update(:architecture, :contacted_at)
+        expect_redis_update
 
         subject
       end
@@ -417,25 +424,23 @@ describe Ci::Runner do
       end
 
       it 'updates database' do
-        expect_redis_update(:architecture, :contacted_at)
+        expect_redis_update
 
         expect { subject }.to change { runner.reload.read_attribute(:contacted_at) }
           .and change { runner.reload.read_attribute(:architecture) }
       end
 
       it 'updates cache' do
-        expect_redis_update(:architecture, :contacted_at)
+        expect_redis_update
 
         subject
       end
     end
 
-    def expect_redis_update(*params)
+    def expect_redis_update
       Gitlab::Redis::SharedState.with do |redis|
-        params.each do |param|
-          redis_key = runner.send(:cache_attribute_key, param)
-          expect(redis).to receive(:set).with(redis_key, anything, any_args)
-        end
+        redis_key = runner.send(:cache_attribute_key)
+        expect(redis).to receive(:set).with(redis_key, anything, any_args)
       end
     end
   end
diff --git a/spec/requests/api/runner_spec.rb b/spec/requests/api/runner_spec.rb
index 0adb314e51d..1040d5dd887 100644
--- a/spec/requests/api/runner_spec.rb
+++ b/spec/requests/api/runner_spec.rb
@@ -409,7 +409,7 @@ describe API::Runner do
             expect { request_job }.to change { runner.reload.contacted_at }
           end
 
-          %w(name version revision platform architecture).each do |param|
+          %w(version revision platform architecture).each do |param|
             context "when info parameter '#{param}' is present" do
               let(:value) { "#{param}_value" }