From 0d7a6e918b2ef7cc202a87ecfe3534cc3167895c Mon Sep 17 00:00:00 2001 From: Marius Bobin <mbobin@gitlab.com> Date: Tue, 19 May 2020 15:14:08 +0300 Subject: [PATCH] Extract process memory caching mechanism into a concern Extract process memory caching mechanism used to cache instance level CI/CD variables into a new concern --- app/models/ci/instance_variable.rb | 42 ++------------- lib/gitlab/process_memory_cache/helper.rb | 51 ++++++++++++++++++ .../process_memory_cache/helper_spec.rb | 52 +++++++++++++++++++ spec/models/ci/instance_variable_spec.rb | 13 +---- 4 files changed, 108 insertions(+), 50 deletions(-) create mode 100644 lib/gitlab/process_memory_cache/helper.rb create mode 100644 spec/lib/gitlab/process_memory_cache/helper_spec.rb diff --git a/app/models/ci/instance_variable.rb b/app/models/ci/instance_variable.rb index c674f76d229..557f3a63280 100644 --- a/app/models/ci/instance_variable.rb +++ b/app/models/ci/instance_variable.rb @@ -3,6 +3,7 @@ module Ci class InstanceVariable < ApplicationRecord extend Gitlab::Ci::Model + extend Gitlab::ProcessMemoryCache::Helper include Ci::NewHasVariable include Ci::Maskable @@ -13,7 +14,8 @@ module Ci } scope :unprotected, -> { where(protected: false) } - after_commit { self.class.touch_redis_cache_timestamp } + + after_commit { self.class.invalidate_memory_cache(:ci_instance_variable_data) } class << self def all_cached @@ -24,10 +26,6 @@ module Ci cached_data[:unprotected] end - def touch_redis_cache_timestamp(time = Time.current.to_f) - shared_backend.write(:ci_instance_variable_changed_at, time) - end - private def cached_data @@ -37,40 +35,6 @@ module Ci { all: all_records, unprotected: all_records.reject(&:protected?) } end end - - def fetch_memory_cache(key, &payload) - cache = process_backend.read(key) - - if cache && !stale_cache?(cache) - cache[:data] - else - store_cache(key, &payload) - end - end - - def stale_cache?(cache_info) - shared_timestamp = shared_backend.read(:ci_instance_variable_changed_at) - return true unless shared_timestamp - - shared_timestamp.to_f > cache_info[:cached_at].to_f - end - - def store_cache(key) - data = yield - time = Time.current.to_f - - process_backend.write(key, data: data, cached_at: time) - touch_redis_cache_timestamp(time) - data - end - - def shared_backend - Rails.cache - end - - def process_backend - Gitlab::ProcessMemoryCache.cache_backend - end end end end diff --git a/lib/gitlab/process_memory_cache/helper.rb b/lib/gitlab/process_memory_cache/helper.rb new file mode 100644 index 00000000000..ee4b81a9a19 --- /dev/null +++ b/lib/gitlab/process_memory_cache/helper.rb @@ -0,0 +1,51 @@ +# frozen_string_literal: true + +module Gitlab + class ProcessMemoryCache + module Helper + def fetch_memory_cache(key, &payload) + cache = cache_backend.read(key) + + if cache && !stale_cache?(key, cache) + cache[:data] + else + store_cache(key, &payload) + end + end + + def invalidate_memory_cache(key) + touch_cache_timestamp(key) + end + + private + + def touch_cache_timestamp(key, time = Time.current.to_f) + shared_backend.write(key, time) + end + + def stale_cache?(key, cache_info) + shared_timestamp = shared_backend.read(key) + return true unless shared_timestamp + + shared_timestamp.to_f > cache_info[:cached_at].to_f + end + + def store_cache(key) + data = yield + time = Time.current.to_f + + cache_backend.write(key, data: data, cached_at: time) + touch_cache_timestamp(key, time) + data + end + + def shared_backend + Rails.cache + end + + def cache_backend + ::Gitlab::ProcessMemoryCache.cache_backend + end + end + end +end diff --git a/spec/lib/gitlab/process_memory_cache/helper_spec.rb b/spec/lib/gitlab/process_memory_cache/helper_spec.rb new file mode 100644 index 00000000000..890642b1d5e --- /dev/null +++ b/spec/lib/gitlab/process_memory_cache/helper_spec.rb @@ -0,0 +1,52 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Gitlab::ProcessMemoryCache::Helper, :use_clean_rails_memory_store_caching do + let(:minimal_test_class) do + Class.new do + include Gitlab::ProcessMemoryCache::Helper + + def cached_content + fetch_memory_cache(:cached_content_instance_key) { expensive_computation } + end + + def clear_cached_content + invalidate_memory_cache(:cached_content_instance_key) + end + end + end + + before do + stub_const("MinimalTestClass", minimal_test_class) + end + + subject { MinimalTestClass.new } + + describe '.fetch_memory_cache' do + it 'memoizes the result' do + is_expected.to receive(:expensive_computation).once.and_return(1) + + 2.times do + expect(subject.cached_content).to eq(1) + end + end + + it 'resets the cache when the shared key is missing', :aggregate_failures do + expect(Rails.cache).to receive(:read).with(:cached_content_instance_key).twice.and_return(nil) + is_expected.to receive(:expensive_computation).thrice.and_return(1, 2, 3) + + 3.times do |index| + expect(subject.cached_content).to eq(index + 1) + end + end + end + + describe '.invalidate_memory_cache' do + it 'invalidates the cache' do + is_expected.to receive(:expensive_computation).twice.and_return(1, 2) + + expect { subject.clear_cached_content }.to change { subject.cached_content } + end + end +end diff --git a/spec/models/ci/instance_variable_spec.rb b/spec/models/ci/instance_variable_spec.rb index ff8676e1424..4ad168ff0f2 100644 --- a/spec/models/ci/instance_variable_spec.rb +++ b/spec/models/ci/instance_variable_spec.rb @@ -39,7 +39,7 @@ describe Ci::InstanceVariable do it { expect(described_class.all_cached).to contain_exactly(protected_variable, unprotected_variable) } it 'memoizes the result' do - expect(described_class).to receive(:store_cache).with(:ci_instance_variable_data).once.and_call_original + expect(described_class).to receive(:unscoped).once.and_call_original 2.times do expect(described_class.all_cached).to contain_exactly(protected_variable, unprotected_variable) @@ -65,15 +65,6 @@ describe Ci::InstanceVariable do expect(described_class.all_cached).to contain_exactly(protected_variable, unprotected_variable, variable) end - - it 'resets the cache when the shared key is missing' do - expect(Rails.cache).to receive(:read).with(:ci_instance_variable_changed_at).twice.and_return(nil) - expect(described_class).to receive(:store_cache).with(:ci_instance_variable_data).thrice.and_call_original - - 3.times do - expect(described_class.all_cached).to contain_exactly(protected_variable, unprotected_variable) - end - end end describe '.unprotected_cached', :use_clean_rails_memory_store_caching do @@ -83,7 +74,7 @@ describe Ci::InstanceVariable do it { expect(described_class.unprotected_cached).to contain_exactly(unprotected_variable) } it 'memoizes the result' do - expect(described_class).to receive(:store_cache).with(:ci_instance_variable_data).once.and_call_original + expect(described_class).to receive(:unscoped).once.and_call_original 2.times do expect(described_class.unprotected_cached).to contain_exactly(unprotected_variable) -- 2.30.9