Merge branch 'sh-add-thread-memory-cache-ee' into 'master'

[EE] Add a memory cache local to the thread to reduce Redis load

Closes gitlab-ce#63977

See merge request gitlab-org/gitlab-ee!14500

app/controllers/admin/application_settings_controller.rb

@@ -64,7 +64,7 @@ class Admin::ApplicationSettingsController < Admin::ApplicationController
   private
 
   def set_application_setting
-    @application_setting = Gitlab::CurrentSettings.current_application_settings
+    @application_setting = ApplicationSetting.current_without_cache
   end
 
   def whitelist_query_limiting

app/helpers/application_settings_helper.rb

@@ -69,7 +69,7 @@ module ApplicationSettingsHelper
   # toggle button effect.
   def import_sources_checkboxes(help_block_id, options = {})
     Gitlab::ImportSources.options.map do |name, source|
-      checked = Gitlab::CurrentSettings.import_sources.include?(source)
+      checked = @application_setting.import_sources.include?(source)
       css_class = checked ? 'active' : ''
       checkbox_name = 'application_setting[import_sources][]'
 
@@ -85,7 +85,7 @@ module ApplicationSettingsHelper
 
   def oauth_providers_checkboxes
     button_based_providers.map do |source|
-      disabled = Gitlab::CurrentSettings.disabled_oauth_sign_in_sources.include?(source.to_s)
+      disabled = @application_setting.disabled_oauth_sign_in_sources.include?(source.to_s)
       css_class = ['btn']
       css_class << 'active' unless disabled
       checkbox_name = 'application_setting[enabled_oauth_sign_in_sources][]'

app/models/application_setting.rb

@@ -272,6 +272,14 @@ class ApplicationSetting < ApplicationRecord
     # We already have an ApplicationSetting record, so just return it.
     current_without_cache
   end
+
+  # By default, the backend is Rails.cache, which uses
+  # ActiveSupport::Cache::RedisStore. Since loading ApplicationSetting
+  # can cause a significant amount of load on Redis, let's cache it in
+  # memory.
+  def self.cache_backend
+    Gitlab::ThreadMemoryCache.cache_backend
+  end
 end
 
 ApplicationSetting.prepend(EE::ApplicationSetting)

app/models/concerns/cacheable_attributes.rb

@@ -36,7 +36,7 @@ module CacheableAttributes
     end
 
     def retrieve_from_cache
-      record = Rails.cache.read(cache_key)
+      record = cache_backend.read(cache_key)
       ensure_cache_setup if record.present?
 
       record
@@ -58,7 +58,7 @@ module CacheableAttributes
     end
 
     def expire
-      Rails.cache.delete(cache_key)
+      cache_backend.delete(cache_key)
     rescue
       # Gracefully handle when Redis is not available. For example,
       # omnibus may fail here during gitlab:assets:compile.
@@ -69,9 +69,13 @@ module CacheableAttributes
       # to be loaded when read from cache: https://github.com/rails/rails/issues/27348
       define_attribute_methods
     end
+
+    def cache_backend
+      Rails.cache
+    end
   end
 
   def cache!
-    Rails.cache.write(self.class.cache_key, self, expires_in: 1.minute)
+    self.class.cache_backend.write(self.class.cache_key, self, expires_in: 1.minute)
   end
 end

app/models/user.rb

@@ -1460,7 +1460,7 @@ class User < ApplicationRecord
   end
 
   def requires_usage_stats_consent?
-    !consented_usage_stats? && 7.days.ago > self.created_at && !has_current_license? && User.single_user?
+    self.admin? && 7.days.ago > self.created_at && !has_current_license? && User.single_user? && !consented_usage_stats?
   end
 
   # Avoid migrations only building user preference object when needed.
@@ -1495,7 +1495,14 @@ class User < ApplicationRecord
   end
 
   def consented_usage_stats?
-    Gitlab::CurrentSettings.usage_stats_set_by_user_id == self.id
+    # Bypass the cache here because it's possible the admin enabled the
+    # usage ping, and we don't want to annoy the user again if they
+    # already set the value. This is a bit of hack, but the alternative
+    # would be to put in a more complex cache invalidation step. Since
+    # this call only gets called in the uncommon situation where the
+    # user is an admin and the only user in the instance, this shouldn't
+    # cause too much load on the system.
+    ApplicationSetting.current_without_cache&.usage_stats_set_by_user_id == self.id
   end
 
   # Added according to https://github.com/plataformatec/devise/blob/7df57d5081f9884849ca15e4fde179ef164a575f/README.md#activejob-integration

changelogs/unreleased/sh-add-thread-memory-cache.yml

+---
+title: Add a memory cache local to the thread to reduce Redis load
+merge_request: 30233
+author:
+type: performance

config/initializers/0_thread_cache.rb

+# frozen_string_literal: true
+
+Gitlab::ThreadMemoryCache.cache_backend

ee/spec/features/admin/admin_settings_spec.rb

@@ -19,8 +19,8 @@ describe 'Admin updates EE-only settings' do
           click_button 'Save changes'
         end
 
-        expect(Gitlab::CurrentSettings.geo_status_timeout).to eq(15)
-        expect(Gitlab::CurrentSettings.geo_node_allowed_ips).to eq('192.34.34.34')
+        expect(current_settings.geo_status_timeout).to eq(15)
+        expect(current_settings.geo_node_allowed_ips).to eq('192.34.34.34')
         expect(page).to have_content "Application settings saved successfully"
       end
     end
@@ -63,10 +63,10 @@ describe 'Admin updates EE-only settings' do
       end
 
       aggregate_failures do
-        expect(Gitlab::CurrentSettings.elasticsearch_indexing).to be_truthy
-        expect(Gitlab::CurrentSettings.elasticsearch_search).to be_truthy
-        expect(Gitlab::CurrentSettings.elasticsearch_shards).to eq(120)
-        expect(Gitlab::CurrentSettings.elasticsearch_replicas).to eq(2)
+        expect(current_settings.elasticsearch_indexing).to be_truthy
+        expect(current_settings.elasticsearch_search).to be_truthy
+        expect(current_settings.elasticsearch_shards).to eq(120)
+        expect(current_settings.elasticsearch_replicas).to eq(2)
         expect(page).to have_content "Application settings saved successfully"
       end
     end
@@ -111,7 +111,7 @@ describe 'Admin updates EE-only settings' do
 
       wait_for_all_requests
 
-      expect(Gitlab::CurrentSettings.elasticsearch_limit_indexing).to be_truthy
+      expect(current_settings.elasticsearch_limit_indexing).to be_truthy
       expect(ElasticsearchIndexedNamespace.exists?(namespace_id: namespace.id)).to be_truthy
       expect(ElasticsearchIndexedProject.exists?(project_id: project.id)).to be_truthy
       expect(page).to have_content "Application settings saved successfully"
@@ -204,4 +204,8 @@ describe 'Admin updates EE-only settings' do
       end
     end
   end
+
+  def current_settings
+    ApplicationSetting.current_without_cache
+  end
 end

lib/gitlab/thread_memory_cache.rb

+# frozen_string_literal: true
+
+module Gitlab
+  class ThreadMemoryCache
+    THREAD_KEY = :thread_memory_cache
+
+    def self.cache_backend
+      # Note ActiveSupport::Cache::MemoryStore is thread-safe.  Since
+      # each backend is local per thread we probably don't need to worry
+      # about synchronizing access, but this is a drop-in replacement
+      # for ActiveSupport::Cache::RedisStore.
+      Thread.current[THREAD_KEY] ||= ActiveSupport::Cache::MemoryStore.new
+    end
+  end
+end

spec/features/admin/admin_settings_spec.rb

@@ -40,7 +40,7 @@ describe 'Admin updates settings' do
     end
 
     it 'Modify import sources' do
-      expect(Gitlab::CurrentSettings.import_sources).not_to be_empty
+      expect(current_settings.import_sources).not_to be_empty
 
       page.within('.as-visibility-access') do
         Gitlab::ImportSources.options.map do |name, _|
@@ -51,7 +51,7 @@ describe 'Admin updates settings' do
       end
 
       expect(page).to have_content "Application settings saved successfully"
-      expect(Gitlab::CurrentSettings.import_sources).to be_empty
+      expect(current_settings.import_sources).to be_empty
 
       page.within('.as-visibility-access') do
         check "Repo by URL"
@@ -59,7 +59,7 @@ describe 'Admin updates settings' do
       end
 
       expect(page).to have_content "Application settings saved successfully"
-      expect(Gitlab::CurrentSettings.import_sources).to eq(['git'])
+      expect(current_settings.import_sources).to eq(['git'])
     end
 
     it 'Change Visibility and Access Controls' do
@@ -68,7 +68,7 @@ describe 'Admin updates settings' do
         click_button 'Save changes'
       end
 
-      expect(Gitlab::CurrentSettings.project_export_enabled).to be_falsey
+      expect(current_settings.project_export_enabled).to be_falsey
       expect(page).to have_content "Application settings saved successfully"
     end
 
@@ -96,7 +96,7 @@ describe 'Admin updates settings' do
         click_button 'Save changes'
       end
 
-      expect(Gitlab::CurrentSettings.gravatar_enabled).to be_falsey
+      expect(current_settings.gravatar_enabled).to be_falsey
       expect(page).to have_content "Application settings saved successfully"
     end
 
@@ -118,7 +118,7 @@ describe 'Admin updates settings' do
         click_button 'Save changes'
       end
 
-      expect(Gitlab::CurrentSettings.home_page_url).to eq "https://about.gitlab.com/"
+      expect(current_settings.home_page_url).to eq "https://about.gitlab.com/"
       expect(page).to have_content "Application settings saved successfully"
     end
 
@@ -133,13 +133,13 @@ describe 'Admin updates settings' do
         click_button 'Save changes'
       end
 
-      expect(Gitlab::CurrentSettings.enforce_terms).to be(true)
-      expect(Gitlab::CurrentSettings.terms).to eq 'Be nice!'
+      expect(current_settings.enforce_terms).to be(true)
+      expect(current_settings.terms).to eq 'Be nice!'
       expect(page).to have_content 'Application settings saved successfully'
     end
 
     it 'Modify oauth providers' do
-      expect(Gitlab::CurrentSettings.disabled_oauth_sign_in_sources).to be_empty
+      expect(current_settings.disabled_oauth_sign_in_sources).to be_empty
 
       page.within('.as-signin') do
         uncheck 'Google'
@@ -147,7 +147,7 @@ describe 'Admin updates settings' do
       end
 
       expect(page).to have_content "Application settings saved successfully"
-      expect(Gitlab::CurrentSettings.disabled_oauth_sign_in_sources).to include('google_oauth2')
+      expect(current_settings.disabled_oauth_sign_in_sources).to include('google_oauth2')
 
       page.within('.as-signin') do
         check "Google"
@@ -155,11 +155,11 @@ describe 'Admin updates settings' do
       end
 
       expect(page).to have_content "Application settings saved successfully"
-      expect(Gitlab::CurrentSettings.disabled_oauth_sign_in_sources).not_to include('google_oauth2')
+      expect(current_settings.disabled_oauth_sign_in_sources).not_to include('google_oauth2')
     end
 
     it 'Oauth providers do not raise validation errors when saving unrelated changes' do
-      expect(Gitlab::CurrentSettings.disabled_oauth_sign_in_sources).to be_empty
+      expect(current_settings.disabled_oauth_sign_in_sources).to be_empty
 
       page.within('.as-signin') do
         uncheck 'Google'
@@ -167,7 +167,7 @@ describe 'Admin updates settings' do
       end
 
       expect(page).to have_content "Application settings saved successfully"
-      expect(Gitlab::CurrentSettings.disabled_oauth_sign_in_sources).to include('google_oauth2')
+      expect(current_settings.disabled_oauth_sign_in_sources).to include('google_oauth2')
 
       # Remove google_oauth2 from the Omniauth strategies
       allow(Devise).to receive(:omniauth_providers).and_return([])
@@ -178,7 +178,7 @@ describe 'Admin updates settings' do
       end
 
       expect(page).to have_content "Application settings saved successfully"
-      expect(Gitlab::CurrentSettings.disabled_oauth_sign_in_sources).to include('google_oauth2')
+      expect(current_settings.disabled_oauth_sign_in_sources).to include('google_oauth2')
     end
 
     it 'Configure web terminal' do
@@ -188,7 +188,7 @@ describe 'Admin updates settings' do
       end
 
       expect(page).to have_content "Application settings saved successfully"
-      expect(Gitlab::CurrentSettings.terminal_max_session_time).to eq(15)
+      expect(current_settings.terminal_max_session_time).to eq(15)
     end
   end
 
@@ -204,7 +204,7 @@ describe 'Admin updates settings' do
       end
 
       expect(page).to have_content "Application settings saved successfully"
-      expect(Gitlab::CurrentSettings.hide_third_party_offers).to be true
+      expect(current_settings.hide_third_party_offers).to be true
     end
 
     it 'Change Slack Notifications Service template settings' do
@@ -249,8 +249,8 @@ describe 'Admin updates settings' do
         click_button 'Save changes'
       end
 
-      expect(Gitlab::CurrentSettings.auto_devops_enabled?).to be true
-      expect(Gitlab::CurrentSettings.auto_devops_domain).to eq('domain.com')
+      expect(current_settings.auto_devops_enabled?).to be true
+      expect(current_settings.auto_devops_domain).to eq('domain.com')
       expect(page).to have_content "Application settings saved successfully"
     end
   end
@@ -268,8 +268,8 @@ describe 'Admin updates settings' do
       end
 
       expect(page).to have_content "Application settings saved successfully"
-      expect(Gitlab::CurrentSettings.recaptcha_enabled).to be true
-      expect(Gitlab::CurrentSettings.unique_ips_limit_per_user).to eq(15)
+      expect(current_settings.recaptcha_enabled).to be true
+      expect(current_settings.unique_ips_limit_per_user).to eq(15)
     end
   end
 
@@ -284,7 +284,7 @@ describe 'Admin updates settings' do
         click_button 'Save changes'
       end
 
-      expect(Gitlab::CurrentSettings.metrics_enabled?).to be true
+      expect(current_settings.metrics_enabled?).to be true
       expect(page).to have_content "Application settings saved successfully"
     end
 
@@ -294,7 +294,7 @@ describe 'Admin updates settings' do
         click_button 'Save changes'
       end
 
-      expect(Gitlab::CurrentSettings.prometheus_metrics_enabled?).to be true
+      expect(current_settings.prometheus_metrics_enabled?).to be true
       expect(page).to have_content "Application settings saved successfully"
     end
 
@@ -343,8 +343,8 @@ describe 'Admin updates settings' do
       end
 
       expect(page).to have_content "Application settings saved successfully"
-      expect(Gitlab::CurrentSettings.allow_local_requests_from_hooks_and_services).to be true
-      expect(Gitlab::CurrentSettings.dns_rebinding_protection_enabled).to be false
+      expect(current_settings.allow_local_requests_from_hooks_and_services).to be true
+      expect(current_settings.dns_rebinding_protection_enabled).to be false
     end
   end
 
@@ -361,9 +361,9 @@ describe 'Admin updates settings' do
         click_button 'Save changes'
       end
 
-      expect(Gitlab::CurrentSettings.help_page_text).to eq "Example text"
-      expect(Gitlab::CurrentSettings.help_page_hide_commercial_content).to be_truthy
-      expect(Gitlab::CurrentSettings.help_page_support_url).to eq "http://example.com/help"
+      expect(current_settings.help_page_text).to eq "Example text"
+      expect(current_settings.help_page_hide_commercial_content).to be_truthy
+      expect(current_settings.help_page_support_url).to eq "http://example.com/help"
       expect(page).to have_content "Application settings saved successfully"
     end
 
@@ -374,8 +374,8 @@ describe 'Admin updates settings' do
         click_button 'Save changes'
       end
 
-      expect(Gitlab::CurrentSettings.max_pages_size).to eq 15
-      expect(Gitlab::CurrentSettings.pages_domain_verification_enabled?).to be_truthy
+      expect(current_settings.max_pages_size).to eq 15
+      expect(current_settings.pages_domain_verification_enabled?).to be_truthy
       expect(page).to have_content "Application settings saved successfully"
     end
 
@@ -385,7 +385,7 @@ describe 'Admin updates settings' do
         click_button 'Save changes'
       end
 
-      expect(Gitlab::CurrentSettings.polling_interval_multiplier).to eq 5.0
+      expect(current_settings.polling_interval_multiplier).to eq 5.0
       expect(page).to have_content "Application settings saved successfully"
     end
 
@@ -395,7 +395,7 @@ describe 'Admin updates settings' do
         click_button 'Save changes'
       end
 
-      expect(Gitlab::CurrentSettings.polling_interval_multiplier).not_to eq(-1.0)
+      expect(current_settings.polling_interval_multiplier).not_to eq(-1.0)
       expect(page)
         .to have_content "The form contains the following error: Polling interval multiplier must be greater than or equal to 0"
     end
@@ -413,8 +413,8 @@ describe 'Admin updates settings' do
           click_button 'Save changes'
         end
 
-        expect(Gitlab::CurrentSettings.lets_encrypt_notification_email).to eq 'my@test.example.com'
-        expect(Gitlab::CurrentSettings.lets_encrypt_terms_of_service_accepted).to eq true
+        expect(current_settings.lets_encrypt_notification_email).to eq 'my@test.example.com'
+        expect(current_settings.lets_encrypt_terms_of_service_accepted).to eq true
       end
     end
 
@@ -445,4 +445,8 @@ describe 'Admin updates settings' do
     page.check('Wiki page')
     page.check('Deployment')
   end
+
+  def current_settings
+    ApplicationSetting.current_without_cache
+  end
 end

spec/features/users/terms_spec.rb

@@ -81,15 +81,18 @@ describe 'Users > Terms' do
 
         enforce_terms
 
-        within('.nav-sidebar') do
-          click_link 'Issues'
-        end
+        # Application settings are cached for a minute
+        Timecop.travel 2.minutes do
+          within('.nav-sidebar') do
+            click_link 'Issues'
+          end
 
-        expect_to_be_on_terms_page
+          expect_to_be_on_terms_page
 
-        click_button('Accept terms')
+          click_button('Accept terms')
 
-        expect(current_path).to eq(project_issues_path(project))
+          expect(current_path).to eq(project_issues_path(project))
+        end
       end
 
       # Disabled until https://gitlab.com/gitlab-org/gitlab-ce/issues/37162 is solved properly

spec/models/concerns/cacheable_attributes_spec.rb

@@ -143,14 +143,14 @@ describe CacheableAttributes do
         allow(ApplicationSetting).to receive(:current_without_cache).twice.and_return(nil)
 
         expect(ApplicationSetting.current).to be_nil
-        expect(Rails.cache.exist?(ApplicationSetting.cache_key)).to be(false)
+        expect(ApplicationSetting.cache_backend.exist?(ApplicationSetting.cache_key)).to be(false)
       end
 
       it 'caches non-nil object' do
         create(:application_setting)
 
         expect(ApplicationSetting.current).to eq(ApplicationSetting.last)
-        expect(Rails.cache.exist?(ApplicationSetting.cache_key)).to be(true)
+        expect(ApplicationSetting.cache_backend.exist?(ApplicationSetting.cache_key)).to be(true)
 
         # subsequent calls retrieve the record from the cache
         last_record = ApplicationSetting.last
@@ -188,11 +188,12 @@ describe CacheableAttributes do
       end
     end
 
-    it 'uses RequestStore in addition to Rails.cache', :request_store do
+    it 'uses RequestStore in addition to Thread memory cache', :request_store do
       # Warm up the cache
       create(:application_setting).cache!
 
-      expect(Rails.cache).to receive(:read).with(ApplicationSetting.cache_key).once.and_call_original
+      expect(ApplicationSetting.cache_backend).to eq(Gitlab::ThreadMemoryCache.cache_backend)
+      expect(ApplicationSetting.cache_backend).to receive(:read).with(ApplicationSetting.cache_key).once.and_call_original
 
       2.times { ApplicationSetting.current }
     end

spec/models/user_spec.rb

@@ -3354,7 +3354,7 @@ describe User do
   end
 
   describe '#requires_usage_stats_consent?' do
-    let(:user) { create(:user, created_at: 8.days.ago) }
+    let(:user) { create(:user, :admin, created_at: 8.days.ago) }
 
     before do
       allow(user).to receive(:has_current_license?).and_return false
@@ -3378,7 +3378,7 @@ describe User do
       end
 
       it 'does not require consent if usage stats were set by this user' do
-        allow(Gitlab::CurrentSettings).to receive(:usage_stats_set_by_user_id).and_return(user.id)
+        create(:application_setting, usage_stats_set_by_user_id: user.id)
 
         expect(user.requires_usage_stats_consent?).to be false
       end

spec/spec_helper.rb

@@ -141,6 +141,8 @@ RSpec.configure do |config|
     allow(Feature).to receive(:enabled?)
       .with(:force_autodevops_on_by_default, anything)
       .and_return(false)
+
+    Gitlab::ThreadMemoryCache.cache_backend.clear
   end
 
   config.around(:example, :quarantine) do