From 388d5473e11f1e1b4119cf55b9f499cf87f87c8b Mon Sep 17 00:00:00 2001 From: David Roetzel Date: Mon, 2 Sep 2024 16:19:55 +0200 Subject: [PATCH] Refactor (ruby) redis configuration (#31694) --- ...s_configuration.rb => redis_connection.rb} | 29 ++- app/models/concerns/redisable.rb | 4 +- config/application.rb | 3 +- config/environments/development.rb | 2 +- config/environments/production.rb | 2 +- config/initializers/sidekiq.rb | 4 +- config/initializers/stoplight.rb | 2 +- .../20170920032311_fix_reblogs_in_feeds.rb | 2 +- ...00407202420_migrate_unavailable_inboxes.rb | 2 +- lib/chewy/strategy/mastodon.rb | 2 +- lib/mastodon/cli/base.rb | 2 +- lib/mastodon/cli/progress_helper.rb | 2 +- lib/mastodon/rack_middleware.rb | 2 +- lib/mastodon/redis_config.rb | 53 ------ lib/mastodon/redis_configuration.rb | 96 ++++++++++ lib/mastodon/sidekiq_middleware.rb | 2 +- spec/lib/mastodon/redis_configuration_spec.rb | 170 ++++++++++++++++++ spec/rails_helper.rb | 2 +- spec/services/resolve_account_service_spec.rb | 2 +- spec/support/streaming_server_manager.rb | 2 +- 20 files changed, 295 insertions(+), 90 deletions(-) rename app/lib/{redis_configuration.rb => redis_connection.rb} (65%) delete mode 100644 lib/mastodon/redis_config.rb create mode 100644 lib/mastodon/redis_configuration.rb create mode 100644 spec/lib/mastodon/redis_configuration_spec.rb diff --git a/app/lib/redis_configuration.rb b/app/lib/redis_connection.rb similarity index 65% rename from app/lib/redis_configuration.rb rename to app/lib/redis_connection.rb index fb1249640f..24e376e6a5 100644 --- a/app/lib/redis_configuration.rb +++ b/app/lib/redis_connection.rb @@ -1,6 +1,6 @@ # frozen_string_literal: true -class RedisConfiguration +class RedisConnection class << self def establish_pool(new_pool_size) @pool&.shutdown(&:close) @@ -22,33 +22,24 @@ class RedisConfiguration end end + attr_reader :config + + def initialize + @config = REDIS_CONFIGURATION.base + end + def connection - if namespace? + namespace = config[:namespace] + if namespace.present? Redis::Namespace.new(namespace, redis: raw_connection) else raw_connection end end - def namespace? - namespace.present? - end - - def namespace - ENV.fetch('REDIS_NAMESPACE', nil) - end - - def url - ENV['REDIS_URL'] - end - - def redis_driver - ENV.fetch('REDIS_DRIVER', 'hiredis') == 'ruby' ? :ruby : :hiredis - end - private def raw_connection - Redis.new(url: url, driver: redis_driver) + Redis.new(**config) end end diff --git a/app/models/concerns/redisable.rb b/app/models/concerns/redisable.rb index 0dad3abb2b..01763fa297 100644 --- a/app/models/concerns/redisable.rb +++ b/app/models/concerns/redisable.rb @@ -2,10 +2,10 @@ module Redisable def redis - Thread.current[:redis] ||= RedisConfiguration.pool.checkout + Thread.current[:redis] ||= RedisConnection.pool.checkout end def with_redis(&block) - RedisConfiguration.with(&block) + RedisConnection.with(&block) end end diff --git a/config/application.rb b/config/application.rb index 3c62a49228..0013c78858 100644 --- a/config/application.rb +++ b/config/application.rb @@ -101,7 +101,8 @@ module Mastodon end config.before_configuration do - require 'mastodon/redis_config' + require 'mastodon/redis_configuration' + ::REDIS_CONFIGURATION = Mastodon::RedisConfiguration.new config.x.use_vips = ENV['MASTODON_USE_LIBVIPS'] == 'true' diff --git a/config/environments/development.rb b/config/environments/development.rb index e4da60ac8e..74f0913da2 100644 --- a/config/environments/development.rb +++ b/config/environments/development.rb @@ -25,7 +25,7 @@ Rails.application.configure do config.action_controller.perform_caching = true config.action_controller.enable_fragment_cache_logging = true - config.cache_store = :redis_cache_store, REDIS_CACHE_PARAMS + config.cache_store = :redis_cache_store, REDIS_CONFIGURATION.cache config.public_file_server.headers = { 'Cache-Control' => "public, max-age=#{2.days.to_i}", } diff --git a/config/environments/production.rb b/config/environments/production.rb index b42f78b14e..5129b73e41 100644 --- a/config/environments/production.rb +++ b/config/environments/production.rb @@ -57,7 +57,7 @@ Rails.application.configure do config.log_tags = [:request_id] # Use a different cache store in production. - config.cache_store = :redis_cache_store, REDIS_CACHE_PARAMS + config.cache_store = :redis_cache_store, REDIS_CONFIGURATION.cache # Use a real queuing backend for Active Job (and separate queues per environment). # config.active_job.queue_adapter = :resque diff --git a/config/initializers/sidekiq.rb b/config/initializers/sidekiq.rb index 5b2f317bf2..5b281c4339 100644 --- a/config/initializers/sidekiq.rb +++ b/config/initializers/sidekiq.rb @@ -3,7 +3,7 @@ require_relative '../../lib/mastodon/sidekiq_middleware' Sidekiq.configure_server do |config| - config.redis = REDIS_SIDEKIQ_PARAMS + config.redis = REDIS_CONFIGURATION.sidekiq # This is used in Kubernetes setups, to signal that the Sidekiq process has started and will begin processing jobs # This comes from https://github.com/sidekiq/sidekiq/wiki/Kubernetes#sidekiq @@ -51,7 +51,7 @@ Sidekiq.configure_server do |config| end Sidekiq.configure_client do |config| - config.redis = REDIS_SIDEKIQ_PARAMS + config.redis = REDIS_CONFIGURATION.sidekiq config.client_middleware do |chain| chain.add SidekiqUniqueJobs::Middleware::Client diff --git a/config/initializers/stoplight.rb b/config/initializers/stoplight.rb index 7c13d50637..0ade504f67 100644 --- a/config/initializers/stoplight.rb +++ b/config/initializers/stoplight.rb @@ -3,6 +3,6 @@ require 'stoplight' Rails.application.reloader.to_prepare do - Stoplight.default_data_store = Stoplight::DataStore::Redis.new(RedisConfiguration.new.connection) + Stoplight.default_data_store = Stoplight::DataStore::Redis.new(RedisConnection.new.connection) Stoplight.default_notifiers = [Stoplight::Notifier::Logger.new(Rails.logger)] end diff --git a/db/migrate/20170920032311_fix_reblogs_in_feeds.rb b/db/migrate/20170920032311_fix_reblogs_in_feeds.rb index fd6ad39f08..e752915fb9 100644 --- a/db/migrate/20170920032311_fix_reblogs_in_feeds.rb +++ b/db/migrate/20170920032311_fix_reblogs_in_feeds.rb @@ -2,7 +2,7 @@ class FixReblogsInFeeds < ActiveRecord::Migration[5.1] def up - redis = RedisConfiguration.pool.checkout + redis = RedisConnection.pool.checkout fm = FeedManager.instance # Old scheme: diff --git a/db/migrate/20200407202420_migrate_unavailable_inboxes.rb b/db/migrate/20200407202420_migrate_unavailable_inboxes.rb index a79045839b..3caacbe90b 100644 --- a/db/migrate/20200407202420_migrate_unavailable_inboxes.rb +++ b/db/migrate/20200407202420_migrate_unavailable_inboxes.rb @@ -4,7 +4,7 @@ class MigrateUnavailableInboxes < ActiveRecord::Migration[5.2] disable_ddl_transaction! def up - redis = RedisConfiguration.pool.checkout + redis = RedisConnection.pool.checkout urls = redis.smembers('unavailable_inboxes') hosts = urls.filter_map do |url| diff --git a/lib/chewy/strategy/mastodon.rb b/lib/chewy/strategy/mastodon.rb index ee8b921865..a4b655c506 100644 --- a/lib/chewy/strategy/mastodon.rb +++ b/lib/chewy/strategy/mastodon.rb @@ -14,7 +14,7 @@ module Chewy end def leave - RedisConfiguration.with do |redis| + RedisConnection.with do |redis| redis.pipelined do |pipeline| @stash.each do |type, ids| pipeline.sadd("chewy:queue:#{type.name}", ids) diff --git a/lib/mastodon/cli/base.rb b/lib/mastodon/cli/base.rb index 93dec1fb8f..dfdcec57be 100644 --- a/lib/mastodon/cli/base.rb +++ b/lib/mastodon/cli/base.rb @@ -40,7 +40,7 @@ module Mastodon .dup .tap { |config| config['pool'] = options[:concurrency] + 1 } ) - RedisConfiguration.establish_pool(options[:concurrency]) + RedisConnection.establish_pool(options[:concurrency]) end end end diff --git a/lib/mastodon/cli/progress_helper.rb b/lib/mastodon/cli/progress_helper.rb index f22492afcf..5634343796 100644 --- a/lib/mastodon/cli/progress_helper.rb +++ b/lib/mastodon/cli/progress_helper.rb @@ -51,7 +51,7 @@ module Mastodon::CLI result = ActiveRecord::Base.connection_pool.with_connection do yield(item) ensure - RedisConfiguration.pool.checkin if Thread.current[:redis] + RedisConnection.pool.checkin if Thread.current[:redis] Thread.current[:redis] = nil end diff --git a/lib/mastodon/rack_middleware.rb b/lib/mastodon/rack_middleware.rb index 8aa7911fe7..0e452f06d6 100644 --- a/lib/mastodon/rack_middleware.rb +++ b/lib/mastodon/rack_middleware.rb @@ -19,7 +19,7 @@ class Mastodon::RackMiddleware end def clean_up_redis_socket! - RedisConfiguration.pool.checkin if Thread.current[:redis] + RedisConnection.pool.checkin if Thread.current[:redis] Thread.current[:redis] = nil end diff --git a/lib/mastodon/redis_config.rb b/lib/mastodon/redis_config.rb deleted file mode 100644 index c858b61a05..0000000000 --- a/lib/mastodon/redis_config.rb +++ /dev/null @@ -1,53 +0,0 @@ -# frozen_string_literal: true - -def setup_redis_env_url(prefix = nil, defaults = true) - prefix = "#{prefix.to_s.upcase}_" unless prefix.nil? - prefix = '' if prefix.nil? - - return if ENV["#{prefix}REDIS_URL"].present? - - password = ENV.fetch("#{prefix}REDIS_PASSWORD") { '' if defaults } - host = ENV.fetch("#{prefix}REDIS_HOST") { 'localhost' if defaults } - port = ENV.fetch("#{prefix}REDIS_PORT") { 6379 if defaults } - db = ENV.fetch("#{prefix}REDIS_DB") { 0 if defaults } - - ENV["#{prefix}REDIS_URL"] = begin - if [password, host, port, db].all?(&:nil?) - ENV['REDIS_URL'] - else - Addressable::URI.parse("redis://#{host}:#{port}/#{db}").tap do |uri| - uri.password = password if password.present? - end.normalize.to_str - end - end -end - -setup_redis_env_url -setup_redis_env_url(:cache, false) -setup_redis_env_url(:sidekiq, false) - -namespace = ENV.fetch('REDIS_NAMESPACE', nil) -cache_namespace = namespace ? "#{namespace}_cache" : 'cache' -sidekiq_namespace = namespace - -redis_driver = ENV.fetch('REDIS_DRIVER', 'hiredis') == 'ruby' ? :ruby : :hiredis - -REDIS_CACHE_PARAMS = { - driver: redis_driver, - url: ENV['CACHE_REDIS_URL'], - expires_in: 10.minutes, - namespace: "#{cache_namespace}:7.1", - connect_timeout: 5, - pool: { - size: Sidekiq.server? ? Sidekiq[:concurrency] : Integer(ENV['MAX_THREADS'] || 5), - timeout: 5, - }, -}.freeze - -REDIS_SIDEKIQ_PARAMS = { - driver: redis_driver, - url: ENV['SIDEKIQ_REDIS_URL'], - namespace: sidekiq_namespace, -}.freeze - -ENV['REDIS_NAMESPACE'] = "mastodon_test#{ENV['TEST_ENV_NUMBER']}" if Rails.env.test? diff --git a/lib/mastodon/redis_configuration.rb b/lib/mastodon/redis_configuration.rb new file mode 100644 index 0000000000..3cd121e4ac --- /dev/null +++ b/lib/mastodon/redis_configuration.rb @@ -0,0 +1,96 @@ +# frozen_string_literal: true + +class Mastodon::RedisConfiguration + def base + @base ||= { + url: setup_base_redis_url, + driver: driver, + namespace: base_namespace, + } + end + + def sidekiq + @sidekiq ||= { + url: setup_prefixed_redis_url(:sidekiq), + driver: driver, + namespace: sidekiq_namespace, + } + end + + def cache + @cache ||= { + url: setup_prefixed_redis_url(:cache), + driver: driver, + namespace: cache_namespace, + expires_in: 10.minutes, + connect_timeout: 5, + pool: { + size: Sidekiq.server? ? Sidekiq[:concurrency] : Integer(ENV['MAX_THREADS'] || 5), + timeout: 5, + }, + } + end + + private + + def driver + ENV['REDIS_DRIVER'] == 'ruby' ? :ruby : :hiredis + end + + def namespace + @namespace ||= ENV.fetch('REDIS_NAMESPACE', nil) + end + + def base_namespace + return "mastodon_test#{ENV.fetch('TEST_ENV_NUMBER', nil)}" if Rails.env.test? + + namespace + end + + def sidekiq_namespace + namespace + end + + def cache_namespace + namespace ? "#{namespace}_cache" : 'cache' + end + + def setup_base_redis_url + url = ENV.fetch('REDIS_URL', nil) + return url if url.present? + + user = ENV.fetch('REDIS_USER', '') + password = ENV.fetch('REDIS_PASSWORD', '') + host = ENV.fetch('REDIS_HOST', 'localhost') + port = ENV.fetch('REDIS_PORT', 6379) + db = ENV.fetch('REDIS_DB', 0) + + construct_uri(host, port, db, user, password) + end + + def setup_prefixed_redis_url(prefix) + prefix = "#{prefix.to_s.upcase}_" + url = ENV.fetch("#{prefix}REDIS_URL", nil) + + return url if url.present? + + user = ENV.fetch("#{prefix}REDIS_USER", nil) + password = ENV.fetch("#{prefix}REDIS_PASSWORD", nil) + host = ENV.fetch("#{prefix}REDIS_HOST", nil) + port = ENV.fetch("#{prefix}REDIS_PORT", nil) + db = ENV.fetch("#{prefix}REDIS_DB", nil) + + if host.nil? + base[:url] + else + construct_uri(host, port, db, user, password) + end + end + + def construct_uri(host, port, db, user, password) + Addressable::URI.parse("redis://#{host}:#{port}/#{db}").tap do |uri| + uri.user = user if user.present? + uri.password = password if password.present? + end.normalize.to_str + end +end diff --git a/lib/mastodon/sidekiq_middleware.rb b/lib/mastodon/sidekiq_middleware.rb index c5f4d8da35..8ce1124c69 100644 --- a/lib/mastodon/sidekiq_middleware.rb +++ b/lib/mastodon/sidekiq_middleware.rb @@ -53,7 +53,7 @@ class Mastodon::SidekiqMiddleware end def clean_up_redis_socket! - RedisConfiguration.pool.checkin if Thread.current[:redis] + RedisConnection.pool.checkin if Thread.current[:redis] Thread.current[:redis] = nil end diff --git a/spec/lib/mastodon/redis_configuration_spec.rb b/spec/lib/mastodon/redis_configuration_spec.rb new file mode 100644 index 0000000000..c7326fd411 --- /dev/null +++ b/spec/lib/mastodon/redis_configuration_spec.rb @@ -0,0 +1,170 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe Mastodon::RedisConfiguration do + let(:redis_environment) { described_class.new } + + before do + # We use one numbered namespace per parallel test runner + # in the test env. This here should test the non-test + # behavior, so we disable it temporarily. + allow(Rails.env).to receive(:test?).and_return(false) + end + + shared_examples 'setting a different driver' do + context 'when setting the `REDIS_DRIVER` variable to `ruby`' do + around do |example| + ClimateControl.modify REDIS_DRIVER: 'ruby' do + example.run + end + end + + it 'sets the driver accordingly' do + expect(subject[:driver]).to eq :ruby + end + end + end + + shared_examples 'setting a namespace' do + context 'when setting the `REDIS_NAMESPACE` variable' do + around do |example| + ClimateControl.modify REDIS_NAMESPACE: 'testns' do + example.run + end + end + + it 'uses the value for the namespace' do + expect(subject[:namespace]).to eq 'testns' + end + end + end + + shared_examples 'secondary configuration' do |prefix| + context "when no `#{prefix}_REDIS_` environment variables are present" do + it 'uses the url from the base config' do + expect(subject[:url]).to eq 'redis://localhost:6379/0' + end + end + + context "when the `#{prefix}_REDIS_URL` environment variable is present" do + around do |example| + ClimateControl.modify "#{prefix}_REDIS_URL": 'redis::/user@other.example.com/4' do + example.run + end + end + + it 'uses the provided URL' do + expect(subject[:url]).to eq 'redis::/user@other.example.com/4' + end + end + + context 'when giving separate environment variables' do + around do |example| + ClimateControl.modify "#{prefix}_REDIS_PASSWORD": 'testpass1', "#{prefix}_REDIS_HOST": 'redis2.example.com', "#{prefix}_REDIS_PORT": '3322', "#{prefix}_REDIS_DB": '8' do + example.run + end + end + + it 'constructs the url from them' do + expect(subject[:url]).to eq 'redis://:testpass1@redis2.example.com:3322/8' + end + end + end + + describe '#base' do + subject { redis_environment.base } + + context 'when no `REDIS_` environment variables are present' do + it 'uses defaults' do + expect(subject).to eq({ + url: 'redis://localhost:6379/0', + driver: :hiredis, + namespace: nil, + }) + end + end + + context 'when the `REDIS_URL` environment variable is present' do + around do |example| + ClimateControl.modify REDIS_URL: 'redis::/user@example.com/2' do + example.run + end + end + + it 'uses the provided URL' do + expect(subject).to eq({ + url: 'redis::/user@example.com/2', + driver: :hiredis, + namespace: nil, + }) + end + end + + context 'when giving separate environment variables' do + around do |example| + ClimateControl.modify REDIS_PASSWORD: 'testpass', REDIS_HOST: 'redis.example.com', REDIS_PORT: '3333', REDIS_DB: '3' do + example.run + end + end + + it 'constructs the url from them' do + expect(subject).to eq({ + url: 'redis://:testpass@redis.example.com:3333/3', + driver: :hiredis, + namespace: nil, + }) + end + end + + include_examples 'setting a different driver' + include_examples 'setting a namespace' + end + + describe '#sidekiq' do + subject { redis_environment.sidekiq } + + include_examples 'secondary configuration', 'SIDEKIQ' + include_examples 'setting a different driver' + include_examples 'setting a namespace' + end + + describe '#cache' do + subject { redis_environment.cache } + + it 'includes extra configuration' do + expect(subject).to eq({ + url: 'redis://localhost:6379/0', + driver: :hiredis, + namespace: 'cache', + expires_in: 10.minutes, + connect_timeout: 5, + pool: { + size: 5, + timeout: 5, + }, + }) + end + + context 'when `REDIS_NAMESPACE` is not set' do + it 'uses the `cache` namespace' do + expect(subject[:namespace]).to eq 'cache' + end + end + + context 'when setting the `REDIS_NAMESPACE` variable' do + around do |example| + ClimateControl.modify REDIS_NAMESPACE: 'testns' do + example.run + end + end + + it 'attaches the `_cache` postfix to the namespace' do + expect(subject[:namespace]).to eq 'testns_cache' + end + end + + include_examples 'secondary configuration', 'CACHE' + include_examples 'setting a different driver' + end +end diff --git a/spec/rails_helper.rb b/spec/rails_helper.rb index 9af2d1e337..ba712c08f9 100644 --- a/spec/rails_helper.rb +++ b/spec/rails_helper.rb @@ -176,5 +176,5 @@ def stub_reset_connection_pools # TODO: Is there a better way to correctly run specs without stubbing this? # (Avoids reset_connection_pools! in test env) allow(ActiveRecord::Base).to receive(:establish_connection) - allow(RedisConfiguration).to receive(:establish_pool) + allow(RedisConnection).to receive(:establish_pool) end diff --git a/spec/services/resolve_account_service_spec.rb b/spec/services/resolve_account_service_spec.rb index e0084a1579..a856e019a7 100644 --- a/spec/services/resolve_account_service_spec.rb +++ b/spec/services/resolve_account_service_spec.rb @@ -228,7 +228,7 @@ RSpec.describe ResolveAccountService do rescue ActiveRecord::RecordNotUnique fail_occurred = true ensure - RedisConfiguration.pool.checkin if Thread.current[:redis] + RedisConnection.pool.checkin if Thread.current[:redis] end end diff --git a/spec/support/streaming_server_manager.rb b/spec/support/streaming_server_manager.rb index 376d6b8725..78cadcf6ad 100644 --- a/spec/support/streaming_server_manager.rb +++ b/spec/support/streaming_server_manager.rb @@ -17,7 +17,7 @@ class StreamingServerManager @running_thread = Thread.new do Open3.popen2e( { - 'REDIS_NAMESPACE' => ENV.fetch('REDIS_NAMESPACE'), + 'REDIS_NAMESPACE' => REDIS_CONFIGURATION.base[:namespace], 'DB_NAME' => "#{ENV.fetch('DB_NAME', 'mastodon')}_test#{ENV.fetch('TEST_ENV_NUMBER', '')}", 'RAILS_ENV' => ENV.fetch('RAILS_ENV', 'test'), 'NODE_ENV' => ENV.fetch('STREAMING_NODE_ENV', 'development'),