Commit 769a9c30 authored by Rémy Coutable's avatar Rémy Coutable

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

Add a memory cache local to the thread to reduce Redis load

Closes #63977

See merge request gitlab-org/gitlab-ce!30233
parents 29b8830b 618fbde2
......@@ -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
......
......@@ -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][]'
......
......@@ -272,4 +272,12 @@ 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
......@@ -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
......@@ -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
......
---
title: Add a memory cache local to the thread to reduce Redis load
merge_request: 30233
author:
type: performance
# frozen_string_literal: true
Gitlab::ThreadMemoryCache.cache_backend
# 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
......@@ -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
......@@ -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
......
......@@ -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
......
......@@ -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
......
......@@ -139,6 +139,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
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment