diff options
author | Rémy Coutable <remy@rymai.me> | 2019-07-02 10:10:12 +0000 |
---|---|---|
committer | Rémy Coutable <remy@rymai.me> | 2019-07-02 10:10:12 +0000 |
commit | 769a9c300a7473dad9c68e8c0448013f803b496a (patch) | |
tree | 5d4a74fefda8f0bb0424be3cc7f4cfd1c0d1bc6a | |
parent | 29b8830bf8ab7cfe37bc0f41066400509fff519f (diff) | |
parent | 618fbde2b7934ad53e585820ee8adb562a837d7f (diff) | |
download | gitlab-ce-769a9c300a7473dad9c68e8c0448013f803b496a.tar.gz |
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
-rw-r--r-- | app/controllers/admin/application_settings_controller.rb | 2 | ||||
-rw-r--r-- | app/helpers/application_settings_helper.rb | 4 | ||||
-rw-r--r-- | app/models/application_setting.rb | 8 | ||||
-rw-r--r-- | app/models/concerns/cacheable_attributes.rb | 10 | ||||
-rw-r--r-- | app/models/user.rb | 11 | ||||
-rw-r--r-- | changelogs/unreleased/sh-add-thread-memory-cache.yml | 5 | ||||
-rw-r--r-- | config/initializers/0_thread_cache.rb | 3 | ||||
-rw-r--r-- | lib/gitlab/thread_memory_cache.rb | 15 | ||||
-rw-r--r-- | spec/features/admin/admin_settings_spec.rb | 70 | ||||
-rw-r--r-- | spec/features/users/terms_spec.rb | 15 | ||||
-rw-r--r-- | spec/models/concerns/cacheable_attributes_spec.rb | 9 | ||||
-rw-r--r-- | spec/models/user_spec.rb | 4 | ||||
-rw-r--r-- | spec/spec_helper.rb | 2 |
13 files changed, 105 insertions, 53 deletions
diff --git a/app/controllers/admin/application_settings_controller.rb b/app/controllers/admin/application_settings_controller.rb index 42634bf611e..a570da61d54 100644 --- a/app/controllers/admin/application_settings_controller.rb +++ b/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 diff --git a/app/helpers/application_settings_helper.rb b/app/helpers/application_settings_helper.rb index aaaa954047f..a7a4e945a99 100644 --- a/app/helpers/application_settings_helper.rb +++ b/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][]' diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb index fbd8036653a..8e558487c1c 100644 --- a/app/models/application_setting.rb +++ b/app/models/application_setting.rb @@ -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 diff --git a/app/models/concerns/cacheable_attributes.rb b/app/models/concerns/cacheable_attributes.rb index 3d60f6924c1..8cbf4bcfaf7 100644 --- a/app/models/concerns/cacheable_attributes.rb +++ b/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 diff --git a/app/models/user.rb b/app/models/user.rb index 38cb4d1a6e8..26be197209a 100644 --- a/app/models/user.rb +++ b/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 diff --git a/changelogs/unreleased/sh-add-thread-memory-cache.yml b/changelogs/unreleased/sh-add-thread-memory-cache.yml new file mode 100644 index 00000000000..025ad6d9f14 --- /dev/null +++ b/changelogs/unreleased/sh-add-thread-memory-cache.yml @@ -0,0 +1,5 @@ +--- +title: Add a memory cache local to the thread to reduce Redis load +merge_request: 30233 +author: +type: performance diff --git a/config/initializers/0_thread_cache.rb b/config/initializers/0_thread_cache.rb new file mode 100644 index 00000000000..feb8057132e --- /dev/null +++ b/config/initializers/0_thread_cache.rb @@ -0,0 +1,3 @@ +# frozen_string_literal: true + +Gitlab::ThreadMemoryCache.cache_backend diff --git a/lib/gitlab/thread_memory_cache.rb b/lib/gitlab/thread_memory_cache.rb new file mode 100644 index 00000000000..7f363dc7feb --- /dev/null +++ b/lib/gitlab/thread_memory_cache.rb @@ -0,0 +1,15 @@ +# 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 diff --git a/spec/features/admin/admin_settings_spec.rb b/spec/features/admin/admin_settings_spec.rb index 45ef5d07ff0..4a9037afb43 100644 --- a/spec/features/admin/admin_settings_spec.rb +++ b/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 diff --git a/spec/features/users/terms_spec.rb b/spec/features/users/terms_spec.rb index 84df1016594..a770309e6b0 100644 --- a/spec/features/users/terms_spec.rb +++ b/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 diff --git a/spec/models/concerns/cacheable_attributes_spec.rb b/spec/models/concerns/cacheable_attributes_spec.rb index 394fac52aa7..eeacdadab9c 100644 --- a/spec/models/concerns/cacheable_attributes_spec.rb +++ b/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 diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index b098fe3c9f4..a4d177da0be 100644 --- a/spec/models/user_spec.rb +++ b/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 diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 3bd2408dc72..62fdc039b5e 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -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 |