Improve multi store

- Improve multi store specs
- Remove sessions redis instance from build script

lib/gitlab/redis/multi_store.rb

@@ -3,23 +3,20 @@
 module Gitlab
   module Redis
     class MultiStore
+      include Gitlab::Utils::StrongMemoize
+
       class ReadFromPrimaryError < StandardError
         def message
           'Value not found on the redis primary store. Read from the redis secondary store successful.'
         end
       end
-      class MultiReadError < StandardError
-        def message
-          'Value not found on both primary and secondary store.'
-        end
-      end
       class MethodMissingError < StandardError
         def message
           'Method missing. Falling back to execute method on the redis secondary store.'
         end
       end
 
-      attr_reader :primary_store, :secondary_store
+      attr_reader :primary_store, :secondary_store, :instance_name
 
       FAILED_TO_READ_ERROR_MESSAGE = 'Failed to read from the redis primary_store.'
       FAILED_TO_WRITE_ERROR_MESSAGE = 'Failed to write to the redis primary_store.'
@@ -42,18 +39,13 @@ module Gitlab
         flushdb
       ).freeze
 
-      def initialize(primary_store_options, secondary_store_options)
-        @primary_store = ::Redis::Store.new(primary_store_options)
-        @secondary_store = ::Redis::Store.new(secondary_store_options)
-      end
-
-      # This is needed because of Redis::Rack::Connection is requiring Redis::Store
-      # https://github.com/redis-store/redis-rack/blob/a833086ba494083b6a384a1a4e58b36573a9165d/lib/redis/rack/connection.rb#L15
-      # Done similarly in https://github.com/lsegal/yard/blob/main/lib/yard/templates/template.rb#L122
-      def is_a?(klass)
-        return true if klass == ::Redis::Store
+      def initialize(primary_store, secondary_store, instance_name = nil)
+        raise ArgumentError, 'primary_store is required' unless primary_store
+        raise ArgumentError, 'secondary_store is required' unless secondary_store
 
-        super(klass)
+        @primary_store = primary_store
+        @secondary_store = secondary_store
+        @instance_name = instance_name
       end
 
       READ_COMMANDS.each do |name|
@@ -76,23 +68,43 @@ module Gitlab
         end
       end
 
-      def method_missing(command_name, *args, &block)
-        if @instance
-          send_command(@instance, command_name, *args, &block)
-        else
-          log_error(MethodMissingError.new, command_name)
-          increment_method_missing_count(command_name)
+      def method_missing(...)
+        return @instance.send(...) if @instance
 
-          secondary_store.send(command_name, *args, &block) # rubocop:disable GitlabSecurity/PublicSend
-        end
+        log_method_missing(...)
+
+        secondary_store.send(...) # rubocop:disable GitlabSecurity/PublicSend
       end
 
       def respond_to_missing?(command_name, include_private = false)
         true
       end
 
+      # This is needed because of Redis::Rack::Connection is requiring Redis::Store
+      # https://github.com/redis-store/redis-rack/blob/a833086ba494083b6a384a1a4e58b36573a9165d/lib/redis/rack/connection.rb#L15
+      # Done similarly in https://github.com/lsegal/yard/blob/main/lib/yard/templates/template.rb#L122
+      def is_a?(klass)
+        return true if klass == secondary_store.class
+
+        super(klass)
+      end
+      alias_method :kind_of?, :is_a?
+
+      def to_s
+        if multi_store_enabled?
+          primary_store.to_s
+        else
+          secondary_store.to_s
+        end
+      end
+
       private
 
+      def log_method_missing(command_name, *_args)
+        log_error(MethodMissingError.new, command_name)
+        increment_method_missing_count(command_name)
+      end
+
       def read_command(command_name, *args, &block)
         if @instance
           send_command(@instance, command_name, *args, &block)
@@ -128,8 +140,6 @@ module Gitlab
         if value
           log_error(ReadFromPrimaryError.new, command_name)
           increment_read_fallback_count(command_name)
-        else
-          log_error(MultiReadError.new, command_name)
         end
 
         value
@@ -147,15 +157,22 @@ module Gitlab
       end
 
       def multi_store_enabled?
-        Feature.enabled?(:use_multi_store, default_enabled: :yaml)
+        Feature.enabled?(:use_multi_store, default_enabled: :yaml) && !same_redis_store?
+      end
+
+      def same_redis_store?
+        strong_memoize(:same_redis_store) do
+          # <Redis client v4.4.0 for redis:///path_to/redis/redis.socket/5>"
+          primary_store.inspect == secondary_store.inspect
+        end
       end
 
       # rubocop:disable GitlabSecurity/PublicSend
       def send_command(redis_instance, command_name, *args, &block)
         if block_given?
           # Make sure that block is wrapped and executed only on the redis instance that is executing the block
-          redis_instance.send(command_name, *args) do |*args|
-            with_instance(redis_instance, *args, &block)
+          redis_instance.send(command_name, *args) do |*params|
+            with_instance(redis_instance, *params, &block)
           end
         else
           redis_instance.send(command_name, *args)
@@ -163,28 +180,29 @@ module Gitlab
       end
       # rubocop:enable GitlabSecurity/PublicSend
 
-      def with_instance(instance, *args)
+      def with_instance(instance, *params)
         @instance = instance
-        yield(*args)
+
+        yield(*params)
       ensure
         @instance = nil
       end
 
       def increment_read_fallback_count(command_name)
         @read_fallback_counter ||= Gitlab::Metrics.counter(:gitlab_redis_multi_store_read_fallback_total, 'Client side Redis MultiStore reading fallback')
-        @read_fallback_counter.increment(command: command_name)
+        @read_fallback_counter.increment(command: command_name, instance_name: instance_name)
       end
 
       def increment_method_missing_count(command_name)
         @method_missing_counter ||= Gitlab::Metrics.counter(:gitlab_redis_multi_store_method_missing_total, 'Client side Redis MultiStore method missing')
-        @method_missing_counter.increment(command: command_name)
+        @method_missing_counter.increment(command: command_name, innamece_name: instance_name)
       end
 
       def log_error(exception, command_name, extra = {})
         Gitlab::ErrorTracking.log_exception(
           exception,
           command_name: command_name,
-          extra: extra)
+          extra: extra.merge(instance_name: instance_name))
       end
     end
   end

scripts/prepare_build.sh

@@ -38,9 +38,6 @@ sed -i 's|url:.*$|url: redis://redis:6379|g' config/cable.yml
 cp config/resque.yml.example config/resque.yml
 sed -i 's|url:.*$|url: redis://redis:6379|g' config/resque.yml
 
-cp config/resque.yml.example config/redis.sessions.yml
-sed -i 's|url:.*$|url: redis://redis:6379/15|g' config/redis.sessions.yml
-
 if [ "$SETUP_DB" != "false" ]; then
   setup_db
 elif getent hosts postgres; then

spec/lib/gitlab/redis/multi_store_spec.rb

@@ -5,9 +5,25 @@ require 'spec_helper'
 RSpec.describe Gitlab::Redis::MultiStore do
   using RSpec::Parameterized::TableSyntax
 
-  let_it_be(:multi_store) { described_class.new(Gitlab::Redis::Sessions.params.merge(serializer: nil), Gitlab::Redis::SharedState.params.merge(serializer: nil))}
-  let_it_be(:primary_store) { multi_store.primary_store }
-  let_it_be(:secondary_store) { multi_store.secondary_store }
+  let_it_be(:redis_store_class) do
+    Class.new(Gitlab::Redis::Wrapper) do
+      def config_file_name
+        config_file_name = "spec/fixtures/config/redis_new_format_host.yml"
+        Rails.root.join(config_file_name).to_s
+      end
+
+      def self.name
+        'Sessions'
+      end
+    end
+  end
+
+  let_it_be(:primary_db) { 1 }
+  let_it_be(:secondary_db) { 2 }
+  let_it_be(:primary_store) { create_redis_store(redis_store_class.params, db: primary_db, serializer: nil) }
+  let_it_be(:secondary_store) { create_redis_store(redis_store_class.params, db: secondary_db, serializer: nil) }
+  let_it_be(:instance_name) { 'TestStore' }
+  let_it_be(:multi_store) { described_class.new(primary_store, secondary_store, instance_name)}
 
   subject { multi_store.send(name, *args) }
 
@@ -16,6 +32,22 @@ RSpec.describe Gitlab::Redis::MultiStore do
     secondary_store.flushdb
   end
 
+  context 'when primary_store is nil' do
+    let(:multi_store) { described_class.new(nil, secondary_store, instance_name)}
+
+    it 'fails with exception' do
+      expect { multi_store }.to raise_error(ArgumentError, /primary_store is required/)
+    end
+  end
+
+  context 'when secondary_store is nil' do
+    let(:multi_store) { described_class.new(primary_store, nil, instance_name)}
+
+    it 'fails with exception' do
+      expect { multi_store }.to raise_error(ArgumentError, /secondary_store is required/)
+    end
+  end
+
   context 'with READ redis commands' do
     let_it_be(:key1) { "redis:{1}:key_a" }
     let_it_be(:key2) { "redis:{1}:key_b" }
@@ -53,7 +85,7 @@ RSpec.describe Gitlab::Redis::MultiStore do
     RSpec.shared_examples_for 'reads correct value' do
       it 'returns the correct value' do
         if value.is_a?(Array)
-          # :smemebers does not guarantee the order it will return the values (unsorted set)
+          # :smembers does not guarantee the order it will return the values (unsorted set)
           is_expected.to match_array(value)
         else
           is_expected.to eq(value)
@@ -70,7 +102,7 @@ RSpec.describe Gitlab::Redis::MultiStore do
 
       it 'logs the ReadFromPrimaryError' do
         expect(Gitlab::ErrorTracking).to receive(:log_exception).with(an_instance_of(Gitlab::Redis::MultiStore::ReadFromPrimaryError),
-          hash_including(command_name: name))
+          hash_including(command_name: name, extra: hash_including(instance_name: instance_name)))
 
         subject
       end
@@ -83,7 +115,7 @@ RSpec.describe Gitlab::Redis::MultiStore do
 
       include_examples 'reads correct value'
 
-      context 'when fallback read from the secondary instance raises and exception' do
+      context 'when fallback read from the secondary instance raises an exception' do
         before do
           allow(secondary_store).to receive(name).with(*args).and_raise(StandardError)
         end
@@ -92,24 +124,21 @@ RSpec.describe Gitlab::Redis::MultiStore do
           expect { subject }.to raise_error(StandardError)
         end
       end
+    end
 
-      context 'when fallback read from the secondary instance returns no value' do
-        before do
-          allow(secondary_store).to receive(name).and_return(nil)
-        end
+    RSpec.shared_examples_for 'secondary store' do
+      it 'execute on the secondary instance' do
+        expect(secondary_store).to receive(name).with(*args).and_call_original
 
-        it 'logs the MultiReadError error' do
-          expect(Gitlab::ErrorTracking).to receive(:log_exception).with(an_instance_of(Gitlab::Redis::MultiStore::MultiReadError),
-            hash_including(command_name: name))
+        subject
+      end
 
-          subject
-        end
+      include_examples 'reads correct value'
 
-        it 'does not increment read fallback count metrics' do
-          expect(multi_store).not_to receive(:increment_read_fallback_count)
+      it 'does not execute on the primary store' do
+        expect(primary_store).not_to receive(name)
 
-          subject
-        end
+        subject
       end
     end
 
@@ -149,7 +178,7 @@ RSpec.describe Gitlab::Redis::MultiStore do
 
             it 'logs the exception' do
               expect(Gitlab::ErrorTracking).to receive(:log_exception).with(an_instance_of(StandardError),
-                hash_including(extra: hash_including(:multi_store_error_message),
+                hash_including(extra: hash_including(:multi_store_error_message, instance_name: instance_name),
                                command_name: name))
 
               subject
@@ -202,19 +231,15 @@ RSpec.describe Gitlab::Redis::MultiStore do
             stub_feature_flags(use_multi_store: false)
           end
 
-          it 'execute on the secondary instance' do
-            expect(secondary_store).to receive(name).with(*args).and_call_original
-
-            subject
-          end
-
-          include_examples 'reads correct value'
+          it_behaves_like 'secondary store'
+        end
 
-          it 'does not execute on the primary store' do
-            expect(primary_store).not_to receive(name)
+        context 'with both primary and secondary store using same redis instance' do
+          let(:primary_store) { create_redis_store(redis_store_class.params, db: primary_db, serializer: nil) }
+          let(:secondary_store) { create_redis_store(redis_store_class.params, db: primary_db , serializer: nil) }
+          let(:multi_store) { described_class.new(primary_store, secondary_store, instance_name)}
 
-            subject
-          end
+          it_behaves_like 'secondary store'
         end
       end
     end
@@ -267,7 +292,7 @@ RSpec.describe Gitlab::Redis::MultiStore do
         redis_store = multi_store.send(store)
 
         if expected_value.is_a?(Array)
-          # :smemebers does not guarantee the order it will return the values
+          # :smembers does not guarantee the order it will return the values
           expect(redis_store.send(verification_name, *verification_args)).to match_array(expected_value)
         else
           expect(redis_store.send(verification_name, *verification_args)).to eq(expected_value)
@@ -375,7 +400,8 @@ RSpec.describe Gitlab::Redis::MultiStore do
 
     it 'logs MethodMissingError' do
       expect(Gitlab::ErrorTracking).to receive(:log_exception).with(an_instance_of(Gitlab::Redis::MultiStore::MethodMissingError),
-        hash_including(command_name: :incr))
+        hash_including(command_name: :incr, extra: hash_including(instance_name: instance_name)))
+
       subject
     end
 
@@ -421,4 +447,8 @@ RSpec.describe Gitlab::Redis::MultiStore do
       end
     end
   end
+
+  def create_redis_store(options, extras = {})
+    ::Redis::Store.new(options.merge(extras))
+  end
 end