From b1c40590e7738a782d1295d743a6a3957723c4b3 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= <matteeyah@gmail.com>
Date: Mon, 29 Jan 2018 00:23:36 +0100
Subject: Extend Runner API helpers with cache info storage

---
 lib/api/helpers/runner.rb | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/lib/api/helpers/runner.rb b/lib/api/helpers/runner.rb
index 2cae53dba53..ad8c1c4407c 100644
--- a/lib/api/helpers/runner.rb
+++ b/lib/api/helpers/runner.rb
@@ -27,6 +27,8 @@ module API
       end
 
       def update_runner_info
+        update_runner_info_cache
+
         return unless update_runner?
 
         current_runner.contacted_at = Time.now
@@ -35,13 +37,17 @@ module API
       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)
+        # Use a 1h threshold to prevent beating DB updates.
 
         current_runner.contacted_at.nil? ||
-          (Time.now - current_runner.contacted_at) >= contacted_at_max_age
+          (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)
-- 
cgit v1.2.1


From 0be8b3cdf677f8b20be820ad3f6fdd4b9b08fc56 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= <matteeyah@gmail.com>
Date: Mon, 29 Jan 2018 00:25:13 +0100
Subject: Check cache in Ci::Runner#online?

---
 app/models/ci/runner.rb       |  9 +++++++-
 spec/models/ci/runner_spec.rb | 54 +++++++++++++++++++++++++++++++++----------
 2 files changed, 50 insertions(+), 13 deletions(-)

diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb
index dcbb397fb78..7e616ee9144 100644
--- a/app/models/ci/runner.rb
+++ b/app/models/ci/runner.rb
@@ -88,7 +88,10 @@ module Ci
     end
 
     def online?
-      contacted_at && contacted_at > self.class.contact_time_deadline
+      Gitlab::Redis::SharedState.with do |redis|
+        last_seen = redis.get("#{self.runner_info_key}:contacted_at") || contacted_at
+        last_seen && last_seen > self.class.contact_time_deadline
+      end
     end
 
     def status
@@ -152,6 +155,10 @@ module Ci
       ensure_runner_queue_value == value if value.present?
     end
 
+    def runner_info_key
+      "runner:info:#{self.id}"
+    end
+
     private
 
     def cleanup_runner_queue
diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb
index b2b64e6ff48..fe9e5ec9436 100644
--- a/spec/models/ci/runner_spec.rb
+++ b/spec/models/ci/runner_spec.rb
@@ -95,28 +95,58 @@ describe Ci::Runner do
 
     subject { runner.online? }
 
-    context 'never contacted' do
+    context 'no cache value' do
       before do
-        runner.contacted_at = nil
+        stub_redis("#{runner.runner_info_key}:contacted_at", nil)
       end
 
-      it { is_expected.to be_falsey }
-    end
+      context 'never contacted' do
+        before do
+          runner.contacted_at = nil
+        end
 
-    context 'contacted long time ago time' do
-      before do
-        runner.contacted_at = 1.year.ago
+        it { is_expected.to be_falsey }
+      end
+
+      context 'contacted long time ago time' do
+        before do
+          runner.contacted_at = 1.year.ago
+        end
+
+        it { is_expected.to be_falsey }
       end
 
-      it { is_expected.to be_falsey }
+      context 'contacted 1s ago' do
+        before do
+          runner.contacted_at = 1.second.ago
+        end
+
+        it { is_expected.to be_truthy }
+      end
     end
 
-    context 'contacted 1s ago' do
-      before do
-        runner.contacted_at = 1.second.ago
+    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)
+        end
+
+        it { is_expected.to be_falsey }
+      end
+
+      context 'contacted 1s ago' do
+        before do
+          stub_redis("#{runner.runner_info_key}:contacted_at", 1.second.ago.to_s)
+        end
+
+        it { is_expected.to be_truthy }
       end
+    end
 
-      it { is_expected.to be_truthy }
+    def stub_redis(key, value)
+      redis = double
+      allow(Gitlab::Redis::SharedState).to receive(:with).and_yield(redis)
+      allow(redis).to receive(:get).with(key).and_return(value)
     end
   end
 
-- 
cgit v1.2.1


From 397442a06140a8cf38bebe3f4519311b1448f23d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= <matteeyah@gmail.com>
Date: Mon, 29 Jan 2018 17:21:30 +0100
Subject: Update runner info on all authenticated requests

---
 lib/api/helpers/runner.rb | 2 ++
 lib/api/runner.rb         | 1 -
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/api/helpers/runner.rb b/lib/api/helpers/runner.rb
index ad8c1c4407c..5ac9181f878 100644
--- a/lib/api/helpers/runner.rb
+++ b/lib/api/helpers/runner.rb
@@ -20,6 +20,8 @@ module API
 
       def authenticate_runner!
         forbidden! unless current_runner
+
+        update_runner_info
       end
 
       def current_runner
diff --git a/lib/api/runner.rb b/lib/api/runner.rb
index 80feb629d54..50cb1df92ad 100644
--- a/lib/api/runner.rb
+++ b/lib/api/runner.rb
@@ -78,7 +78,6 @@ module API
       post '/request' do
         authenticate_runner!
         no_content! unless current_runner.active?
-        update_runner_info
 
         if current_runner.runner_queue_value_latest?(params[:last_update])
           header 'X-GitLab-Last-Update', params[:last_update]
-- 
cgit v1.2.1


From 92ac2b9ee68cad8c01a199e6475bbef272818da5 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= <matteeyah@gmail.com>
Date: Mon, 29 Jan 2018 17:24:29 +0100
Subject: Add CHANGELOG entry

---
 ...ancels-stuck-builds-when-per-job-timeout-is-more-than-an-hour.yml | 5 +++++
 1 file changed, 5 insertions(+)
 create mode 100644 changelogs/unreleased/38265-stuckcijobsworker-wrongly-detects-cancels-stuck-builds-when-per-job-timeout-is-more-than-an-hour.yml

diff --git a/changelogs/unreleased/38265-stuckcijobsworker-wrongly-detects-cancels-stuck-builds-when-per-job-timeout-is-more-than-an-hour.yml b/changelogs/unreleased/38265-stuckcijobsworker-wrongly-detects-cancels-stuck-builds-when-per-job-timeout-is-more-than-an-hour.yml
new file mode 100644
index 00000000000..4d8e6acfcb7
--- /dev/null
+++ b/changelogs/unreleased/38265-stuckcijobsworker-wrongly-detects-cancels-stuck-builds-when-per-job-timeout-is-more-than-an-hour.yml
@@ -0,0 +1,5 @@
+---
+title: Update runner info on all authenticated requests
+merge_request: 16756
+author:
+type: changed
-- 
cgit v1.2.1


From bdd3e39b0bd4e8fcec5d6e2d97297840211dd921 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= <matteeyah@gmail.com>
Date: Mon, 29 Jan 2018 20:56:28 +0100
Subject: Move info update implementation to Ci::Runner model

---
 app/models/ci/runner.rb       | 26 +++++++++++++++++++---
 lib/api/helpers/runner.rb     | 27 +---------------------
 spec/models/ci/runner_spec.rb | 52 ++++++++++++++++++++++++++++++++++++++-----
 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) }
 
-- 
cgit v1.2.1


From df2554558123fbfec007418601e2074cf251db46 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= <matteeyah@gmail.com>
Date: Mon, 29 Jan 2018 21:53:37 +0100
Subject: Generelized cached attribute usage in runner

---
 app/models/ci/runner.rb       | 27 +++++++++++++++++++--------
 spec/models/ci/runner_spec.rb | 17 +++++++++++------
 2 files changed, 30 insertions(+), 14 deletions(-)

diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb
index 44be247bcd3..8fe20622723 100644
--- a/app/models/ci/runner.rb
+++ b/app/models/ci/runner.rb
@@ -68,6 +68,10 @@ module Ci
       ONLINE_CONTACT_TIMEOUT.ago
     end
 
+    def cached_contacted_at
+      runner_info_cache(:contacted_at) || self.contacted_at
+    end
+
     def set_default_values
       self.token = SecureRandom.hex(15) if self.token.blank?
     end
@@ -89,10 +93,7 @@ module Ci
     end
 
     def online?
-      Gitlab::Redis::SharedState.with do |redis|
-        last_seen = redis.get("#{runner_info_redis_cache_key}:contacted_at") || contacted_at
-        last_seen && last_seen > self.class.contact_time_deadline
-      end
+      cached_contacted_at && cached_contacted_at > self.class.contact_time_deadline
     end
 
     def status
@@ -157,7 +158,7 @@ module Ci
     end
 
     def update_runner_info(params)
-      update_runner_info_cache
+      update_runner_info_cache(params)
 
       # Use a 1h threshold to prevent beating DB updates.
       return unless self.contacted_at.nil? ||
@@ -184,10 +185,20 @@ module Ci
       "runner:info:#{self.id}"
     end
 
-    def update_runner_info_cache
+    def update_runner_info_cache(params)
+      Gitlab::Redis::SharedState.with do |redis|
+        redis.set("#{runner_info_redis_cache_key}:contacted_at", Time.now)
+
+        params.each do |key, value|
+          redis_key = "#{runner_info_redis_cache_key}:#{key}"
+          redis.set(redis_key, value)
+        end
+      end
+    end
+
+    def runner_info_cache(attribute)
       Gitlab::Redis::SharedState.with do |redis|
-        redis_key = "#{runner_info_redis_cache_key}:contacted_at"
-        redis.set(redis_key, Time.now)
+        redis.get("#{runner_info_redis_cache_key}:#{attribute}")
       end
     end
 
diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb
index 830f7cd68c5..14747a23c82 100644
--- a/spec/models/ci/runner_spec.rb
+++ b/spec/models/ci/runner_spec.rb
@@ -394,7 +394,7 @@ describe Ci::Runner do
   describe '#update_runner_info' do
     let(:runner) { create(:ci_runner) }
 
-    subject { runner.update_runner_info(contacted_at: Time.now) }
+    subject { runner.update_runner_info(name: 'testing_runner') }
 
     context 'when database was updated recently' do
       before do
@@ -402,7 +402,7 @@ describe Ci::Runner do
       end
 
       it 'updates cache' do
-        expect_redis_update
+        expect_redis_update(:contacted_at, :name)
 
         subject
       end
@@ -414,22 +414,27 @@ describe Ci::Runner do
       end
 
       it 'updates database' do
-        expect_redis_update
+        expect_redis_update(:contacted_at, :name)
 
         expect { subject }.to change { runner.reload.contacted_at }
+          .and change { runner.reload.name }
       end
 
       it 'updates cache' do
-        expect_redis_update
+        expect_redis_update(:contacted_at, :name)
 
         subject
       end
     end
 
-    def expect_redis_update
+    def expect_redis_update(*params)
       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)
+
+      params.each do |param|
+        redis_key = "#{runner.send(:runner_info_redis_cache_key)}:#{param}"
+        expect(redis).to receive(:set).with(redis_key, anything)
+      end
     end
   end
 
-- 
cgit v1.2.1


From b88103e4075678d032d7d7350caaece4a3091328 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= <matteeyah@gmail.com>
Date: Mon, 29 Jan 2018 22:38:09 +0100
Subject: Expose Ci::Runner#cached_contacted_at as Time

---
 app/models/ci/runner.rb | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb
index 8fe20622723..7cf36c0bfe0 100644
--- a/app/models/ci/runner.rb
+++ b/app/models/ci/runner.rb
@@ -69,7 +69,11 @@ module Ci
     end
 
     def cached_contacted_at
-      runner_info_cache(:contacted_at) || self.contacted_at
+      if runner_info_cache(:contacted_at)
+        Time.zone.parse(runner_info_cache(:contacted_at))
+      else
+        self.contacted_at
+      end
     end
 
     def set_default_values
-- 
cgit v1.2.1


From 3be6f68a33d163922a78c51928980efc57a3efb5 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= <matteeyah@gmail.com>
Date: Mon, 29 Jan 2018 23:00:39 +0100
Subject: Make Ci::Runner#online? slightly more performant

This is a small refactor to avoid querying Redis when we know there's
nothing in it.
---
 app/models/ci/runner.rb       | 2 +-
 spec/models/ci/runner_spec.rb | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb
index 7cf36c0bfe0..f6d51fabd69 100644
--- a/app/models/ci/runner.rb
+++ b/app/models/ci/runner.rb
@@ -97,7 +97,7 @@ module Ci
     end
 
     def online?
-      cached_contacted_at && cached_contacted_at > self.class.contact_time_deadline
+      contacted_at && cached_contacted_at > self.class.contact_time_deadline
     end
 
     def status
diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb
index 14747a23c82..99b4a82da88 100644
--- a/spec/models/ci/runner_spec.rb
+++ b/spec/models/ci/runner_spec.rb
@@ -128,6 +128,7 @@ describe Ci::Runner do
     context 'with cache value' do
       context 'contacted long time ago time' do
         before do
+          runner.contacted_at = 1.year.ago
           stub_redis_runner_contacted_at(1.year.ago.to_s)
         end
 
@@ -136,6 +137,7 @@ describe Ci::Runner do
 
       context 'contacted 1s ago' do
         before do
+          runner.contacted_at = 50.minutes.ago
           stub_redis_runner_contacted_at(1.second.ago.to_s)
         end
 
-- 
cgit v1.2.1


From 63ecb57f1b956bb7901e27f26f3bc2f3f62bcaa8 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= <matteeyah@gmail.com>
Date: Mon, 29 Jan 2018 23:25:56 +0100
Subject: Handle updating only contacted_at runner cache

---
 app/models/ci/runner.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb
index f6d51fabd69..fb766cb3793 100644
--- a/app/models/ci/runner.rb
+++ b/app/models/ci/runner.rb
@@ -193,7 +193,7 @@ module Ci
       Gitlab::Redis::SharedState.with do |redis|
         redis.set("#{runner_info_redis_cache_key}:contacted_at", Time.now)
 
-        params.each do |key, value|
+        params && params.each do |key, value|
           redis_key = "#{runner_info_redis_cache_key}:#{key}"
           redis.set(redis_key, value)
         end
-- 
cgit v1.2.1


From 126b6bbc7fdeb1afd5b6d29c86051c48a09c4857 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= <matteeyah@gmail.com>
Date: Mon, 29 Jan 2018 23:27:03 +0100
Subject: Use faster model updates in #update_runner_info spec

---
 spec/models/ci/runner_spec.rb | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb
index 99b4a82da88..ab931e5d43c 100644
--- a/spec/models/ci/runner_spec.rb
+++ b/spec/models/ci/runner_spec.rb
@@ -400,7 +400,7 @@ describe Ci::Runner do
 
     context 'when database was updated recently' do
       before do
-        runner.update(contacted_at: Time.now)
+        runner.contacted_at = Time.now
       end
 
       it 'updates cache' do
@@ -412,7 +412,7 @@ describe Ci::Runner do
 
     context 'when database was not updated recently' do
       before do
-        runner.update(contacted_at: 2.hours.ago)
+        runner.contacted_at = 2.hours.ago
       end
 
       it 'updates database' do
-- 
cgit v1.2.1


From 28fd49c1d23f18692b74e4f4e1f21c24c45da3e6 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= <matteeyah@gmail.com>
Date: Mon, 29 Jan 2018 23:31:34 +0100
Subject: Fix Redis leakage in Runner API specs

---
 spec/requests/api/runner_spec.rb | 1 +
 1 file changed, 1 insertion(+)

diff --git a/spec/requests/api/runner_spec.rb b/spec/requests/api/runner_spec.rb
index cb66d23b77c..25d1ac73b9e 100644
--- a/spec/requests/api/runner_spec.rb
+++ b/spec/requests/api/runner_spec.rb
@@ -8,6 +8,7 @@ describe API::Runner do
   before do
     stub_gitlab_calls
     stub_application_setting(runners_registration_token: registration_token)
+    allow_any_instance_of(Ci::Runner).to receive(:update_runner_info_cache)
   end
 
   describe '/api/v4/runners' do
-- 
cgit v1.2.1


From 38c242126d10d0887ca0d3dd8565ef2669dfec46 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= <matteeyah@gmail.com>
Date: Sun, 4 Feb 2018 18:34:21 +0100
Subject: Refactor runner attribute caching implementation

---
 app/models/ci/runner.rb       | 71 +++++++++++++++++++++++++++----------------
 lib/api/helpers/runner.rb     |  2 +-
 spec/models/ci/runner_spec.rb | 32 +++++++++----------
 3 files changed, 61 insertions(+), 44 deletions(-)

diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb
index fb766cb3793..9749e019d3c 100644
--- a/app/models/ci/runner.rb
+++ b/app/models/ci/runner.rb
@@ -5,7 +5,7 @@ module Ci
 
     RUNNER_QUEUE_EXPIRY_TIME = 60.minutes
     ONLINE_CONTACT_TIMEOUT = 1.hour
-    UPDATE_DB_RUNNER_INFO_EVERY = 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
 
@@ -68,12 +68,20 @@ module Ci
       ONLINE_CONTACT_TIMEOUT.ago
     end
 
-    def cached_contacted_at
-      if runner_info_cache(:contacted_at)
-        Time.zone.parse(runner_info_cache(:contacted_at))
-      else
-        self.contacted_at
-      end
+    def contacted_at
+      cached_attribute(:contacted_at) || read_attribute(:contacted_at)
+    end
+
+    def version
+      cached_attribute(:version) || read_attribute(:version)
+    end
+
+    def revision
+      cached_attribute(:revision) || read_attribute(:revision)
+    end
+
+    def architecture
+      cached_attribute(:architecture) || read_attribute(:architecture)
     end
 
     def set_default_values
@@ -97,7 +105,7 @@ module Ci
     end
 
     def online?
-      contacted_at && cached_contacted_at > self.class.contact_time_deadline
+      contacted_at && contacted_at > self.class.contact_time_deadline
     end
 
     def status
@@ -161,16 +169,16 @@ module Ci
       ensure_runner_queue_value == value if value.present?
     end
 
-    def update_runner_info(params)
-      update_runner_info_cache(params)
+    def update_cached_info(values)
+      values = values.slice(:version, :revision, :platform, :architecture)
+      values[:contacted_at] = Time.now
 
-      # 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
+      cache_attributes(values)
 
-      self.contacted_at = Time.now
-      self.assign_attributes(params)
-      self.save if self.changed?
+      if persist_cached_data?
+        self.assign_attributes(values)
+        self.save
+      end
     end
 
     private
@@ -185,24 +193,33 @@ module Ci
       "runner:build_queue:#{self.token}"
     end
 
-    def runner_info_redis_cache_key
-      "runner:info:#{self.id}"
+    def cache_attribute_key(key)
+      "runner:info:#{self.id}:#{key}"
     end
 
-    def update_runner_info_cache(params)
-      Gitlab::Redis::SharedState.with do |redis|
-        redis.set("#{runner_info_redis_cache_key}:contacted_at", Time.now)
+    def persist_cached_data?
+      # Use a random threshold to prevent beating DB updates.
+      # It generates a distribution between [40m, 80m].
 
-        params && params.each do |key, value|
-          redis_key = "#{runner_info_redis_cache_key}:#{key}"
-          redis.set(redis_key, value)
-        end
+      contacted_at_max_age = UPDATE_DB_RUNNER_INFO_EVERY + Random.rand(UPDATE_DB_RUNNER_INFO_EVERY)
+
+      real_contacted_at = read_attribute(:contacted_at)
+      real_contacted_at.nil? ||
+        (Time.now - real_contacted_at) >= contacted_at_max_age
+    end
+
+    def cached_attribute(key)
+      @cached_attributes = {}
+      @cached_attributes[key] ||= Gitlab::Redis::SharedState.with do |redis|
+        redis.get(cache_attribute_key(key))
       end
     end
 
-    def runner_info_cache(attribute)
+    def cache_attributes(values)
       Gitlab::Redis::SharedState.with do |redis|
-        redis.get("#{runner_info_redis_cache_key}:#{attribute}")
+        values.each do |key, value|
+          redis.set(cache_attribute_key(key), value, ex: 24.hours)
+        end
       end
     end
 
diff --git a/lib/api/helpers/runner.rb b/lib/api/helpers/runner.rb
index 8f45cae0e60..87ba40e26e1 100644
--- a/lib/api/helpers/runner.rb
+++ b/lib/api/helpers/runner.rb
@@ -20,7 +20,7 @@ module API
       def authenticate_runner!
         forbidden! unless current_runner
 
-        current_runner.update_runner_info(get_runner_version_from_params)
+        current_runner.update_cached_info(get_runner_version_from_params)
       end
 
       def current_runner
diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb
index ab931e5d43c..19a4e640090 100644
--- a/spec/models/ci/runner_spec.rb
+++ b/spec/models/ci/runner_spec.rb
@@ -146,9 +146,10 @@ describe Ci::Runner do
     end
 
     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("#{runner.send(:runner_info_redis_cache_key)}:contacted_at").and_return(value)
+      Gitlab::Redis::SharedState.with do |redis|
+        cache_key = runner.send(:cache_attribute_key, :contacted_at)
+        allow(redis).to receive(:get).with(cache_key).and_return(value)
+      end
     end
   end
 
@@ -393,10 +394,10 @@ describe Ci::Runner do
     end
   end
 
-  describe '#update_runner_info' do
+  describe '#update_cached_info' do
     let(:runner) { create(:ci_runner) }
 
-    subject { runner.update_runner_info(name: 'testing_runner') }
+    subject { runner.update_cached_info(architecture: '18-bit') }
 
     context 'when database was updated recently' do
       before do
@@ -404,7 +405,7 @@ describe Ci::Runner do
       end
 
       it 'updates cache' do
-        expect_redis_update(:contacted_at, :name)
+        expect_redis_update(:architecture, :contacted_at)
 
         subject
       end
@@ -416,26 +417,25 @@ describe Ci::Runner do
       end
 
       it 'updates database' do
-        expect_redis_update(:contacted_at, :name)
+        expect_redis_update(:architecture, :contacted_at)
 
-        expect { subject }.to change { runner.reload.contacted_at }
-          .and change { runner.reload.name }
+        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(:contacted_at, :name)
+        expect_redis_update(:architecture, :contacted_at)
 
         subject
       end
     end
 
     def expect_redis_update(*params)
-      redis = double
-      expect(Gitlab::Redis::SharedState).to receive(:with).and_yield(redis)
-
-      params.each do |param|
-        redis_key = "#{runner.send(:runner_info_redis_cache_key)}:#{param}"
-        expect(redis).to receive(:set).with(redis_key, anything)
+      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
       end
     end
   end
-- 
cgit v1.2.1


From 35e04fa5d9a24f012eaf2459e48dda990677f0da Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= <matteeyah@gmail.com>
Date: Sun, 4 Feb 2018 19:19:48 +0100
Subject: Expect instead of allow Redis get stub for online? spec

---
 spec/models/ci/runner_spec.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb
index 19a4e640090..41e27c62bd9 100644
--- a/spec/models/ci/runner_spec.rb
+++ b/spec/models/ci/runner_spec.rb
@@ -148,7 +148,7 @@ 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)
-        allow(redis).to receive(:get).with(cache_key).and_return(value)
+        expect(redis).to receive(:get).with(cache_key).and_return(value).at_least(:once)
       end
     end
   end
-- 
cgit v1.2.1


From 053b9d212b8cb39a6808dc5ccf8ea2c1cd5110fa Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= <matteeyah@gmail.com>
Date: Sun, 4 Feb 2018 19:26:54 +0100
Subject: Fix runner cache update stub in API spec

---
 spec/requests/api/runner_spec.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/spec/requests/api/runner_spec.rb b/spec/requests/api/runner_spec.rb
index 25d1ac73b9e..ea5054a3c9e 100644
--- a/spec/requests/api/runner_spec.rb
+++ b/spec/requests/api/runner_spec.rb
@@ -8,7 +8,7 @@ describe API::Runner do
   before do
     stub_gitlab_calls
     stub_application_setting(runners_registration_token: registration_token)
-    allow_any_instance_of(Ci::Runner).to receive(:update_runner_info_cache)
+    allow_any_instance_of(Ci::Runner).to receive(:cache_attributes)
   end
 
   describe '/api/v4/runners' do
-- 
cgit v1.2.1


From 0abce36cd20cdd3579138bee835d28519a5593f2 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= <matteeyah@gmail.com>
Date: Sun, 4 Feb 2018 23:34:42 +0100
Subject: Update list of allowed attribute updates in Runner

---
 app/models/ci/runner.rb | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb
index 9749e019d3c..5711be3ae29 100644
--- a/app/models/ci/runner.rb
+++ b/app/models/ci/runner.rb
@@ -68,8 +68,8 @@ module Ci
       ONLINE_CONTACT_TIMEOUT.ago
     end
 
-    def contacted_at
-      cached_attribute(:contacted_at) || read_attribute(:contacted_at)
+    def name
+      cached_attribute(:name) || read_attribute(:name)
     end
 
     def version
@@ -80,10 +80,18 @@ module Ci
       cached_attribute(:revision) || read_attribute(:revision)
     end
 
+    def platform
+      cached_attribute(:platform) || read_attribute(:platform)
+    end
+
     def architecture
       cached_attribute(:architecture) || read_attribute(:architecture)
     end
 
+    def contacted_at
+      cached_attribute(:contacted_at) || read_attribute(:contacted_at)
+    end
+
     def set_default_values
       self.token = SecureRandom.hex(15) if self.token.blank?
     end
@@ -170,7 +178,7 @@ module Ci
     end
 
     def update_cached_info(values)
-      values = values.slice(:version, :revision, :platform, :architecture)
+      values = values&.slice(:name, :version, :revision, :platform, :architecture) || {}
       values[:contacted_at] = Time.now
 
       cache_attributes(values)
-- 
cgit v1.2.1


From c92e1d731c8e76bcba3532cf51edc3d53abc1e1f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= <matteeyah@gmail.com>
Date: Mon, 5 Feb 2018 16:47:04 +0100
Subject: Improve runner attribute cachine

---
 app/models/ci/runner.rb          | 52 +++++++++++++++++-----------------------
 spec/models/ci/runner_spec.rb    | 25 +++++++++++--------
 spec/requests/api/runner_spec.rb |  2 +-
 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" }
 
-- 
cgit v1.2.1


From aa60c7a2b521d8a30f10fcb31beb0cdd39c5cbbc Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= <matteeyah@gmail.com>
Date: Mon, 5 Feb 2018 17:07:49 +0100
Subject: Extract attribute caching to RedisCacheable concern

---
 app/models/ci/runner.rb                      | 38 ++------------------------
 app/models/concerns/redis_cacheable.rb       | 41 ++++++++++++++++++++++++++++
 spec/models/concerns/redis_cacheable_spec.rb | 34 +++++++++++++++++++++++
 3 files changed, 78 insertions(+), 35 deletions(-)
 create mode 100644 app/models/concerns/redis_cacheable.rb
 create mode 100644 spec/models/concerns/redis_cacheable_spec.rb

diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb
index 6bd90e71bf3..f6db71be438 100644
--- a/app/models/ci/runner.rb
+++ b/app/models/ci/runner.rb
@@ -2,14 +2,13 @@ module Ci
   class Runner < ActiveRecord::Base
     extend Gitlab::Ci::Model
     include Gitlab::SQL::Pattern
-    include Gitlab::Utils::StrongMemoize
+    include RedisCacheable
 
     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
@@ -50,6 +49,8 @@ module Ci
       ref_protected: 1
     }
 
+    cached_attr_reader :version, :revision, :platform, :architecture, :contacted_at
+
     # Searches for runners matching the given query.
     #
     # This method uses ILIKE on PostgreSQL and LIKE on MySQL.
@@ -70,16 +71,6 @@ module Ci
       ONLINE_CONTACT_TIMEOUT.ago
     end
 
-    def self.cached_attr_reader(*attributes)
-      attributes.each do |attribute|
-        define_method("#{attribute}") do
-          cached_attribute(attribute) || read_attribute(attribute)
-        end
-      end
-    end
-
-    cached_attr_reader :version, :revision, :platform, :architecture, :contacted_at
-
     def set_default_values
       self.token = SecureRandom.hex(15) if self.token.blank?
     end
@@ -189,10 +180,6 @@ module Ci
       "runner:build_queue:#{self.token}"
     end
 
-    def cache_attribute_key
-      "runner:info:#{self.id}"
-    end
-
     def persist_cached_data?
       # Use a random threshold to prevent beating DB updates.
       # It generates a distribution between [40m, 80m].
@@ -204,25 +191,6 @@ module Ci
         (Time.now - real_contacted_at) >= contacted_at_max_age
     end
 
-    def cached_attribute(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|
-        redis.set(cache_attribute_key, values.to_json, ex: CACHED_ATTRIBUTES_EXPIRY_TIME)
-      end
-    end
-
     def tag_constraints
       unless has_tags? || run_untagged?
         errors.add(:tags_list,
diff --git a/app/models/concerns/redis_cacheable.rb b/app/models/concerns/redis_cacheable.rb
new file mode 100644
index 00000000000..249ebf9af07
--- /dev/null
+++ b/app/models/concerns/redis_cacheable.rb
@@ -0,0 +1,41 @@
+module RedisCacheable
+  extend ActiveSupport::Concern
+  include Gitlab::Utils::StrongMemoize
+
+  CACHED_ATTRIBUTES_EXPIRY_TIME = 24.hours
+
+  class_methods do
+    def cached_attr_reader(*attributes)
+      attributes.each do |attribute|
+        define_method("#{attribute}") do
+          cached_attribute(attribute) || read_attribute(attribute)
+        end
+      end
+    end
+  end
+
+  def cached_attribute(attribute)
+    (cached_attributes || {})[attribute]
+  end
+
+  def cache_attributes(values)
+    Gitlab::Redis::SharedState.with do |redis|
+      redis.set(cache_attribute_key, values.to_json, ex: CACHED_ATTRIBUTES_EXPIRY_TIME)
+    end
+  end
+
+  private
+
+  def cache_attribute_key
+    "#{self.class.name}:attributes:#{self.id}"
+  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
+end
diff --git a/spec/models/concerns/redis_cacheable_spec.rb b/spec/models/concerns/redis_cacheable_spec.rb
new file mode 100644
index 00000000000..3edb69f4666
--- /dev/null
+++ b/spec/models/concerns/redis_cacheable_spec.rb
@@ -0,0 +1,34 @@
+require 'spec_helper'
+
+describe RedisCacheable do
+  let(:runner) { build(:ci_runner) }
+
+  describe '#cached_attribute' do
+    subject { runner.cached_attribute(:anything) }
+
+    it 'gets the cache attribute' do
+      Gitlab::Redis::SharedState.with do |redis|
+        expect(redis).to receive(:get).with(runner.send(:cache_attribute_key))
+      end
+
+      subject
+    end
+  end
+
+  describe '#cache_attributes' do
+    let(:values) { { name: 'new_name' } }
+
+    subject { runner.cache_attributes(values) }
+
+    it 'sets the cache attributes' do
+      Gitlab::Redis::SharedState.with do |redis|
+        values.each do |key, value|
+          redis_key = runner.send(:cache_attribute_key)
+          expect(redis).to receive(:set).with(redis_key, values.to_json, anything)
+        end
+      end
+
+      subject
+    end
+  end
+end
-- 
cgit v1.2.1


From 8f557206d8e6dacdbbeb9862ea9cc0a3423f458f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= <matteeyah@gmail.com>
Date: Mon, 5 Feb 2018 17:24:07 +0100
Subject: Save runner attributes only if there's changes

---
 app/models/ci/runner.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb
index f6db71be438..13c784bea0d 100644
--- a/app/models/ci/runner.rb
+++ b/app/models/ci/runner.rb
@@ -164,7 +164,7 @@ module Ci
 
       if persist_cached_data?
         self.assign_attributes(values)
-        self.save
+        self.save if self.changed?
       end
     end
 
-- 
cgit v1.2.1


From aab953da7d26e4ac654e50e80beca1083ea4f11f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= <matteeyah@gmail.com>
Date: Mon, 5 Feb 2018 20:43:23 +0100
Subject: Update RedisCacheable#cache_attribute_key

---
 app/models/concerns/redis_cacheable.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/models/concerns/redis_cacheable.rb b/app/models/concerns/redis_cacheable.rb
index 249ebf9af07..b889f4202dc 100644
--- a/app/models/concerns/redis_cacheable.rb
+++ b/app/models/concerns/redis_cacheable.rb
@@ -27,7 +27,7 @@ module RedisCacheable
   private
 
   def cache_attribute_key
-    "#{self.class.name}:attributes:#{self.id}"
+    "cache:#{self.class.name}:#{self.id}:attributes"
   end
 
   def cached_attributes
-- 
cgit v1.2.1


From 2d2a4ede1f09f7ca98508ad586e56a8cb7285fe3 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= <matteeyah@gmail.com>
Date: Tue, 6 Feb 2018 18:42:28 +0100
Subject: Use double instead of runner in RedisCacheable spec

---
 spec/models/concerns/redis_cacheable_spec.rb | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/spec/models/concerns/redis_cacheable_spec.rb b/spec/models/concerns/redis_cacheable_spec.rb
index 3edb69f4666..d6a7d7d1462 100644
--- a/spec/models/concerns/redis_cacheable_spec.rb
+++ b/spec/models/concerns/redis_cacheable_spec.rb
@@ -1,14 +1,18 @@
 require 'spec_helper'
 
 describe RedisCacheable do
-  let(:runner) { build(:ci_runner) }
+  let(:model) { double }
+
+  before do
+    model.extend(described_class)
+  end
 
   describe '#cached_attribute' do
-    subject { runner.cached_attribute(:anything) }
+    subject { model.cached_attribute(:anything) }
 
     it 'gets the cache attribute' do
       Gitlab::Redis::SharedState.with do |redis|
-        expect(redis).to receive(:get).with(runner.send(:cache_attribute_key))
+        expect(redis).to receive(:get).with(model.send(:cache_attribute_key))
       end
 
       subject
@@ -18,12 +22,12 @@ describe RedisCacheable do
   describe '#cache_attributes' do
     let(:values) { { name: 'new_name' } }
 
-    subject { runner.cache_attributes(values) }
+    subject { model.cache_attributes(values) }
 
     it 'sets the cache attributes' do
       Gitlab::Redis::SharedState.with do |redis|
         values.each do |key, value|
-          redis_key = runner.send(:cache_attribute_key)
+          redis_key = model.send(:cache_attribute_key)
           expect(redis).to receive(:set).with(redis_key, values.to_json, anything)
         end
       end
-- 
cgit v1.2.1


From b75fa80eaae13b4474bac5453420988e14903c4f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= <matteeyah@gmail.com>
Date: Tue, 6 Feb 2018 19:01:23 +0100
Subject: Check return value in RedisCacheable#cached_attribute spec

---
 spec/models/concerns/redis_cacheable_spec.rb | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/spec/models/concerns/redis_cacheable_spec.rb b/spec/models/concerns/redis_cacheable_spec.rb
index d6a7d7d1462..ffb35079149 100644
--- a/spec/models/concerns/redis_cacheable_spec.rb
+++ b/spec/models/concerns/redis_cacheable_spec.rb
@@ -8,14 +8,19 @@ describe RedisCacheable do
   end
 
   describe '#cached_attribute' do
-    subject { model.cached_attribute(:anything) }
+    let(:payload) { { attribute: 'value' } }
+
+    subject { model.cached_attribute(payload.keys.first) }
 
     it 'gets the cache attribute' do
+      expect(model).to receive(:cache_attribute_key).and_return('key')
+
       Gitlab::Redis::SharedState.with do |redis|
-        expect(redis).to receive(:get).with(model.send(:cache_attribute_key))
+        expect(redis).to receive(:get).with('key')
+          .and_return(payload.to_json)
       end
 
-      subject
+      expect(subject).to eq(payload.values.first)
     end
   end
 
-- 
cgit v1.2.1


From e27ea805457a3b794d7a8b3b6b0355eddb1c1eca Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= <matteeyah@gmail.com>
Date: Tue, 6 Feb 2018 19:06:00 +0100
Subject: Fix RedisCacheable#cache_attributes spec

---
 spec/models/concerns/redis_cacheable_spec.rb | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/spec/models/concerns/redis_cacheable_spec.rb b/spec/models/concerns/redis_cacheable_spec.rb
index ffb35079149..3d7963120b6 100644
--- a/spec/models/concerns/redis_cacheable_spec.rb
+++ b/spec/models/concerns/redis_cacheable_spec.rb
@@ -5,6 +5,7 @@ describe RedisCacheable do
 
   before do
     model.extend(described_class)
+    allow(model).to receive(:cache_attribute_key).and_return('key')
   end
 
   describe '#cached_attribute' do
@@ -13,8 +14,6 @@ describe RedisCacheable do
     subject { model.cached_attribute(payload.keys.first) }
 
     it 'gets the cache attribute' do
-      expect(model).to receive(:cache_attribute_key).and_return('key')
-
       Gitlab::Redis::SharedState.with do |redis|
         expect(redis).to receive(:get).with('key')
           .and_return(payload.to_json)
@@ -31,10 +30,7 @@ describe RedisCacheable do
 
     it 'sets the cache attributes' do
       Gitlab::Redis::SharedState.with do |redis|
-        values.each do |key, value|
-          redis_key = model.send(:cache_attribute_key)
-          expect(redis).to receive(:set).with(redis_key, values.to_json, anything)
-        end
+        expect(redis).to receive(:set).with('key', values.to_json, anything)
       end
 
       subject
-- 
cgit v1.2.1