Cache Geo checks for a certain time period instead of per request

Before these checks would always check Redis once per request, and the
Redis key had a timeout of 15 seconds. However, this polling can add
significant load to the Redis server. Instead, revise the caching
strategy instead to the following:

* L1 cache (thead-local, memory store): 1 minute
* L2 cache (Redis): 2 minutes

The downside is cache invalidation: it will take up to 2 minutes for any
changes in the Geo nodes to propagate to a secondary.

Relates to https://gitlab.com/gitlab-com/gl-infra/production/issues/928

Closes https://gitlab.com/gitlab-org/gitlab-ee/issues/12591

ee/changelogs/unreleased/sh-cache-geo-check-in-memory.yml

+---
+title: Cache Geo checks for a certain time period instead of per request
+merge_request: 14513
+author:
+type: performance

ee/lib/gitlab/geo.rb

@@ -81,29 +81,26 @@ module Gitlab
       end
     end
 
-    def self.cache
-      @cache ||= Gitlab::JsonCache.new(namespace: :geo)
+    def self.l1_cache
+      SafeRequestStore[:geo_l1_cache] ||=
+        Gitlab::JsonCache.new(namespace: :geo, backend: ::Gitlab::ThreadMemoryCache.cache_backend)
     end
 
-    def self.request_store_cache
-      Gitlab::SafeRequestStore
+    def self.l2_cache
+      SafeRequestStore[:geo_l2_cache] ||= Gitlab::JsonCache.new(namespace: :geo)
     end
 
     def self.cache_value(raw_key, as: nil, &block)
-      return yield unless request_store_cache.active?
-
-      request_store_cache.fetch(cache.cache_key(raw_key)) do
       # We need a short expire time as we can't manually expire on a secondary node
-        cache.fetch(raw_key, as: as, expires_in: 15.seconds) { yield }
+      l1_cache.fetch(raw_key, as: as, expires_in: 1.minute) do
+        l2_cache.fetch(raw_key, as: as, expires_in: 2.minutes) { yield }
       end
     end
 
     def self.expire_cache!
-      return true unless request_store_cache.active?
-
       CACHE_KEYS.each do |raw_key|
-        cache.expire(raw_key)
-        request_store_cache.delete(cache.cache_key(raw_key))
+        l1_cache.expire(raw_key)
+        l2_cache.expire(raw_key)
       end
 
       true

ee/spec/lib/gitlab/geo_spec.rb

@@ -11,10 +11,10 @@ describe Gitlab::Geo, :geo, :request_store do
     it 'includes GitLab version and Rails.version in the cache key' do
       expanded_key = "geo:#{key}:#{Gitlab::VERSION}:#{Rails.version}"
 
-      expect(Gitlab::SafeRequestStore).to receive(:fetch)
-        .with(expanded_key).and_call_original
+      expect(Gitlab::ThreadMemoryCache.cache_backend).to receive(:write)
+        .with(expanded_key, an_instance_of(String), expires_in: 1.minute).and_call_original
       expect(Rails.cache).to receive(:write)
-        .with(expanded_key, an_instance_of(String), expires_in: 15.seconds)
+        .with(expanded_key, an_instance_of(String), expires_in: 2.minutes)
 
       described_class.public_send(method)
     end
@@ -114,16 +114,6 @@ describe Gitlab::Geo, :geo, :request_store do
         expect(described_class.enabled?).to be_falsey
       end
     end
-
-    context 'with RequestStore enabled', :request_store do
-      it 'return false when no GeoNode exists' do
-        GeoNode.delete_all
-
-        expect(GeoNode).to receive(:exists?).once.and_call_original
-
-        2.times { expect(described_class.enabled?).to be_falsey }
-      end
-    end
   end
 
   describe '.oauth_authentication' do
@@ -177,8 +167,8 @@ describe Gitlab::Geo, :geo, :request_store do
       described_class::CACHE_KEYS.each do |raw_key|
         expanded_key = "geo:#{raw_key}:#{Gitlab::VERSION}:#{Rails.version}"
 
-        expect(Rails.cache).to receive(:delete).with(expanded_key)
-        expect(Gitlab::SafeRequestStore).to receive(:delete).with(expanded_key)
+        expect(Rails.cache).to receive(:delete).with(expanded_key).and_call_original
+        expect(Gitlab::ThreadMemoryCache.cache_backend).to receive(:delete).with(expanded_key).and_call_original
       end
 
       described_class.expire_cache!

ee/spec/workers/geo/migrated_local_files_clean_up_worker_spec.rb

@@ -146,6 +146,9 @@ describe Geo::MigratedLocalFilesCleanUpWorker, :geo, :geo_fdw do
 
     before do
       stub_uploads_object_storage(AvatarUploader)
+
+      allow(Rails.cache).to receive(:read).and_call_original
+      allow(Rails.cache).to receive(:write).and_call_original
     end
 
     it 'sets the back off time when there are no pending items' do

ee/spec/workers/geo/repository_shard_sync_worker_spec.rb

@@ -266,6 +266,11 @@ describe Geo::RepositoryShardSyncWorker, :geo, :geo_fdw, :clean_gitlab_redis_cac
     context 'backoff time' do
       let(:cache_key) { "#{described_class.name.underscore}:shard:#{shard_name}:skip" }
 
+      before do
+        allow(Rails.cache).to receive(:read).and_call_original
+        allow(Rails.cache).to receive(:write).and_call_original
+      end
+
       it 'sets the back off time when there are no pending items' do
         create(:geo_project_registry, :synced, project: unsynced_project_in_restricted_group)
         create(:geo_project_registry, :synced, project: unsynced_project)

ee/spec/workers/geo/repository_verification/primary/shard_worker_spec.rb

@@ -191,6 +191,11 @@ describe Geo::RepositoryVerification::Primary::ShardWorker, :postgresql, :clean_
     context 'backoff time' do
       let(:cache_key) { "#{described_class.name.underscore}:shard:#{shard_name}:skip" }
 
+      before do
+        allow(Rails.cache).to receive(:read).and_call_original
+        allow(Rails.cache).to receive(:write).and_call_original
+      end
+
       it 'sets the back off time when there are no pending items' do
         expect(Rails.cache).to receive(:write).with(cache_key, true, expires_in: 300.seconds).once
 

ee/spec/workers/geo/repository_verification/secondary/shard_worker_spec.rb

 require 'spec_helper'
 
-describe Geo::RepositoryVerification::Secondary::ShardWorker, :geo, :geo_fdw, :clean_gitlab_redis_cache do
+describe Geo::RepositoryVerification::Secondary::ShardWorker, :geo, :geo_fdw, :request_store, :clean_gitlab_redis_cache do
   include ::EE::GeoHelpers
   include ExclusiveLeaseHelpers
 
@@ -187,6 +187,11 @@ describe Geo::RepositoryVerification::Secondary::ShardWorker, :geo, :geo_fdw, :c
     context 'backoff time' do
       let(:cache_key) { "#{described_class.name.underscore}:shard:#{shard_name}:skip" }
 
+      before do
+        allow(Rails.cache).to receive(:write).and_call_original
+        allow(Rails.cache).to receive(:read).and_call_original
+      end
+
       it 'sets the back off time when there are no pending items' do
         expect(Rails.cache).to receive(:write).with(cache_key, true, expires_in: 300.seconds).once
 

spec/lib/gitlab/import_export/relation_rename_service_spec.rb

@@ -28,6 +28,7 @@ describe Gitlab::ImportExport::RelationRenameService do
 
     before do
       allow(shared).to receive(:export_path).and_return(import_path)
+      allow(ActiveSupport::JSON).to receive(:decode).and_call_original
       allow(ActiveSupport::JSON).to receive(:decode).with(file_content).and_return(json_file)
     end