Merge branch '1334-add-use-primary-store-feature-flag' into 'master'

[Follow up] Add use_pirmary_store_as_default feature flag for MultiStore [RUN ALL RSPEC]

See merge request gitlab-org/gitlab!75258

config/feature_flags/development/use_multi_store.yml → config/feature_flags/development/use_primary_and_secondary_stores_for_sessions.yml

 ---
-name: use_multi_store
+name: use_primary_and_secondary_stores_for_sessions
 introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/73660
 rollout_issue_url: https://gitlab.com/gitlab-com/gl-infra/scalability/-/issues/1429
-milestone: '14.5'
+milestone: '14.6'
 type: development
 group: group::memory
 default_enabled: false

config/feature_flags/development/use_primary_store_as_default_for_sessions.yml

+---
+name: use_primary_store_as_default_for_sessions
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/75258
+rollout_issue_url:
+milestone: '14.6'
+type: development
+group: group::memory
+default_enabled: false

lib/gitlab/redis/multi_store.rb

@@ -39,41 +39,42 @@ module Gitlab
         flushdb
       ).freeze
 
-      def initialize(primary_store, secondary_store, instance_name = nil)
+      def initialize(primary_store, secondary_store, instance_name)
         @primary_store = primary_store
         @secondary_store = secondary_store
         @instance_name = instance_name
 
         validate_stores!
       end
-
+      # rubocop:disable GitlabSecurity/PublicSend
       READ_COMMANDS.each do |name|
         define_method(name) do |*args, &block|
-          if multi_store_enabled?
+          if use_primary_and_secondary_stores?
             read_command(name, *args, &block)
           else
-            secondary_store.send(name, *args, &block) # rubocop:disable GitlabSecurity/PublicSend
+            default_store.send(name, *args, &block)
           end
         end
       end
 
       WRITE_COMMANDS.each do |name|
         define_method(name) do |*args, &block|
-          if multi_store_enabled?
+          if use_primary_and_secondary_stores?
             write_command(name, *args, &block)
           else
-            secondary_store.send(name, *args, &block) # rubocop:disable GitlabSecurity/PublicSend
+            default_store.send(name, *args, &block)
           end
         end
       end
 
       def method_missing(...)
-        return @instance.send(...) if @instance # rubocop:disable GitlabSecurity/PublicSend
+        return @instance.send(...) if @instance
 
         log_method_missing(...)
 
-        secondary_store.send(...) # rubocop:disable GitlabSecurity/PublicSend
+        default_store.send(...)
       end
+      # rubocop:enable GitlabSecurity/PublicSend
 
       def respond_to_missing?(command_name, include_private = false)
         true
@@ -83,22 +84,30 @@ module Gitlab
       # 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
+        return true if klass == default_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
+        use_primary_and_secondary_stores? ? primary_store.to_s : default_store.to_s
       end
+
+      def use_primary_and_secondary_stores?
+        Feature.enabled?("use_primary_and_secondary_stores_for_#{instance_name.underscore}", default_enabled: :yaml) && !same_redis_store?
+      end
+
+      def use_primary_store_as_default?
+        Feature.enabled?("use_primary_store_as_default_for_#{instance_name.underscore}", default_enabled: :yaml) && !same_redis_store?
       end
 
       private
 
+      def default_store
+        use_primary_store_as_default? ? primary_store : secondary_store
+      end
+
       def log_method_missing(command_name, *_args)
         log_error(MethodMissingError.new, command_name)
         increment_method_missing_count(command_name)
@@ -155,10 +164,6 @@ module Gitlab
         send_command(secondary_store, command_name, *args, &block)
       end
 
-      def multi_store_enabled?
-        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>"
@@ -200,6 +205,7 @@ module Gitlab
       def validate_stores!
         raise ArgumentError, 'primary_store is required' unless primary_store
         raise ArgumentError, 'secondary_store is required' unless secondary_store
+        raise ArgumentError, 'instance_name is required' unless instance_name
         raise ArgumentError, 'invalid primary_store' unless primary_store.is_a?(::Redis)
         raise ArgumentError, 'invalid secondary_store' unless secondary_store.is_a?(::Redis)
       end

lib/gitlab/redis/sessions.rb

@@ -24,7 +24,7 @@ module Gitlab
           primary_store = ::Redis.new(params)
           secondary_store = ::Redis.new(config_fallback.params)
 
-          MultiStore.new(primary_store, secondary_store, name)
+          MultiStore.new(primary_store, secondary_store, store_name)
         end
       end
 
@@ -35,7 +35,7 @@ module Gitlab
         primary_store = create_redis_store(redis_store_options, extras)
         secondary_store = create_redis_store(self.class.config_fallback.params, extras)
 
-        MultiStore.new(primary_store, secondary_store, self.class.name)
+        MultiStore.new(primary_store, secondary_store, self.class.store_name)
       end
 
       private

spec/lib/gitlab/redis/multi_store_spec.rb

@@ -27,6 +27,11 @@ RSpec.describe Gitlab::Redis::MultiStore do
 
   subject { multi_store.send(name, *args) }
 
+  before do
+    skip_feature_flags_yaml_validation
+    skip_default_enabled_yaml_check
+  end
+
   after(:all) do
     primary_store.flushdb
     secondary_store.flushdb
@@ -48,6 +53,15 @@ RSpec.describe Gitlab::Redis::MultiStore do
     end
   end
 
+  context 'when instance_name is nil' do
+    let(:instance_name) { nil }
+    let(:multi_store) { described_class.new(primary_store, secondary_store, instance_name)}
+
+    it 'fails with exception' do
+      expect { multi_store }.to raise_error(ArgumentError, /instance_name is required/)
+    end
+  end
+
   context 'when primary_store is not a ::Redis instance' do
     before do
       allow(primary_store).to receive(:is_a?).with(::Redis).and_return(false)
@@ -175,9 +189,9 @@ RSpec.describe Gitlab::Redis::MultiStore do
           allow(secondary_store).to receive(name).and_call_original
         end
 
-        context 'with feature flag :use_multi_store enabled' do
+        context 'with feature flag :use_primary_and_secondary_stores_for_test_store' do
           before do
-            stub_feature_flags(use_multi_store: true)
+            stub_feature_flags(use_primary_and_secondary_stores_for_test_store: true)
           end
 
           context 'when reading from the primary is successful' do
@@ -252,14 +266,40 @@ RSpec.describe Gitlab::Redis::MultiStore do
           end
         end
 
-        context 'with feature flag :use_multi_store is disabled' do
+        context 'with feature flag :use_primary_and_secondary_stores_for_test_store' do
+          before do
+            stub_feature_flags(use_primary_and_secondary_stores_for_test_store: false)
+          end
+
+          context 'with feature flag :use_primary_store_as_default_for_test_store is disabled' do
             before do
-            stub_feature_flags(use_multi_store: false)
+              stub_feature_flags(use_primary_store_as_default_for_test_store: false)
             end
 
             it_behaves_like 'secondary store'
           end
 
+          context 'with feature flag :use_primary_store_as_default_for_test_store is enabled' do
+            before do
+              stub_feature_flags(use_primary_store_as_default_for_test_store: true)
+            end
+
+            it 'execute on the primary instance' do
+              expect(primary_store).to receive(name).with(*args).and_call_original
+
+              subject
+            end
+
+            include_examples 'reads correct value'
+
+            it 'does not execute on the secondary store' do
+              expect(secondary_store).not_to receive(name)
+
+              subject
+            end
+          end
+        end
+
         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) }
@@ -335,9 +375,9 @@ RSpec.describe Gitlab::Redis::MultiStore do
           allow(secondary_store).to receive(name).and_call_original
         end
 
-        context 'with feature flag :use_multi_store enabled' do
+        context 'with feature flag :use_primary_and_secondary_stores_for_test_store' do
           before do
-            stub_feature_flags(use_multi_store: true)
+            stub_feature_flags(use_primary_and_secondary_stores_for_test_store: true)
           end
 
           context 'when executing on primary instance is successful' do
@@ -388,9 +428,14 @@ RSpec.describe Gitlab::Redis::MultiStore do
           end
         end
 
-        context 'with feature flag :use_multi_store is disabled' do
+        context 'with feature flag :use_primary_and_secondary_stores_for_test_store is disabled' do
+          before do
+            stub_feature_flags(use_primary_and_secondary_stores_for_test_store: false)
+          end
+
+          context 'with feature flag :use_primary_store_as_default_for_test_store is disabled' do
             before do
-            stub_feature_flags(use_multi_store: false)
+              stub_feature_flags(use_primary_store_as_default_for_test_store: false)
             end
 
             it 'executes only on the secondary redis store', :aggregate_errors do
@@ -402,6 +447,22 @@ RSpec.describe Gitlab::Redis::MultiStore do
 
             include_examples 'verify that store contains values', :secondary_store
           end
+
+          context 'with feature flag :use_primary_store_as_default_for_test_store is enabled' do
+            before do
+              stub_feature_flags(use_primary_store_as_default_for_test_store: true)
+            end
+
+            it 'executes only on the primary_redis redis store', :aggregate_errors do
+              expect(primary_store).to receive(name).with(*expected_args)
+              expect(secondary_store).not_to receive(name).with(*expected_args)
+
+              subject
+            end
+
+            include_examples 'verify that store contains values', :primary_store
+          end
+        end
       end
     end
   end
@@ -440,6 +501,31 @@ RSpec.describe Gitlab::Redis::MultiStore do
       subject
     end
 
+    context 'with feature flag :use_primary_store_as_default_for_test_store is enabled' do
+      before do
+        stub_feature_flags(use_primary_store_as_default_for_test_store: true)
+      end
+
+      it 'fallback and executes only on the secondary store', :aggregate_errors do
+        expect(primary_store).to receive(:incr).with(key).and_call_original
+        expect(secondary_store).not_to receive(:incr)
+
+        subject
+      end
+
+      it 'correct value is stored on the secondary store', :aggregate_errors do
+        subject
+
+        expect(secondary_store.get(key)).to be_nil
+        expect(primary_store.get(key)).to eq('1')
+      end
+    end
+
+    context 'with feature flag :use_primary_store_as_default_for_test_store is disabled' do
+      before do
+        stub_feature_flags(use_primary_store_as_default_for_test_store: false)
+      end
+
       it 'fallback and executes only on the secondary store', :aggregate_errors do
         expect(secondary_store).to receive(:incr).with(key).and_call_original
         expect(primary_store).not_to receive(:incr)
@@ -453,6 +539,7 @@ RSpec.describe Gitlab::Redis::MultiStore do
         expect(primary_store.get(key)).to be_nil
         expect(secondary_store.get(key)).to eq('1')
       end
+    end
 
     context 'when the command is executed within pipelined block' do
       subject do
@@ -477,6 +564,96 @@ RSpec.describe Gitlab::Redis::MultiStore do
     end
   end
 
+  describe '#to_s' do
+    subject { multi_store.to_s }
+
+    context 'with feature flag :use_primary_and_secondary_stores_for_test_store is enabled' do
+      before do
+        stub_feature_flags(use_primary_and_secondary_stores_for_test_store: true)
+      end
+
+      it 'returns same value as primary_store' do
+        is_expected.to eq(primary_store.to_s)
+      end
+    end
+
+    context 'with feature flag :use_primary_and_secondary_stores_for_test_store is disabled' do
+      before do
+        stub_feature_flags(use_primary_and_secondary_stores_for_test_store: false)
+      end
+
+      context 'with feature flag :use_primary_store_as_default_for_test_store is enabled' do
+        before do
+          stub_feature_flags(use_primary_store_as_default_for_test_store: true)
+        end
+
+        it 'returns same value as primary_store' do
+          is_expected.to eq(primary_store.to_s)
+        end
+      end
+
+      context 'with feature flag :use_primary_store_as_default_for_test_store is disabled' do
+        before do
+          stub_feature_flags(use_primary_store_as_default_for_test_store: false)
+        end
+
+        it 'returns same value as primary_store' do
+          is_expected.to eq(secondary_store.to_s)
+        end
+      end
+    end
+  end
+
+  describe '#is_a?' do
+    it 'returns true for ::Redis::Store' do
+      expect(multi_store.is_a?(::Redis::Store)).to be true
+    end
+  end
+
+  describe '#use_primary_and_secondary_stores?' do
+    context 'with feature flag :use_primary_and_secondary_stores_for_test_store is enabled' do
+      before do
+        stub_feature_flags(use_primary_and_secondary_stores_for_test_store: true)
+      end
+
+      it 'multi store is disabled' do
+        expect(multi_store.use_primary_and_secondary_stores?).to be true
+      end
+    end
+
+    context 'with feature flag :use_primary_and_secondary_stores_for_test_store is disabled' do
+      before do
+        stub_feature_flags(use_primary_and_secondary_stores_for_test_store: false)
+      end
+
+      it 'multi store is disabled' do
+        expect(multi_store.use_primary_and_secondary_stores?).to be false
+      end
+    end
+  end
+
+  describe '#use_primary_store_as_default?' do
+    context 'with feature flag :use_primary_store_as_default_for_test_store is enabled' do
+      before do
+        stub_feature_flags(use_primary_store_as_default_for_test_store: true)
+      end
+
+      it 'multi store is disabled' do
+        expect(multi_store.use_primary_store_as_default?).to be true
+      end
+    end
+
+    context 'with feature flag :use_primary_store_as_default_for_test_store is disabled' do
+      before do
+        stub_feature_flags(use_primary_store_as_default_for_test_store: false)
+      end
+
+      it 'multi store is disabled' do
+        expect(multi_store.use_primary_store_as_default?).to be false
+      end
+    end
+  end
+
   def create_redis_store(options, extras = {})
     ::Redis::Store.new(options.merge(extras))
   end

spec/lib/gitlab/redis/sessions_spec.rb

@@ -53,5 +53,7 @@ RSpec.describe Gitlab::Redis::Sessions do
         expect(subject).to be_instance_of(::Gitlab::Redis::MultiStore)
       end
     end
+
+    it_behaves_like 'multi store feature flags', :use_primary_and_secondary_stores_for_sessions, :use_primary_store_as_default_for_sessions
   end
 end

spec/support/shared_examples/lib/gitlab/redis/multi_store_feature_flags_shared_examples.rb

+# frozen_string_literal: true
+
+RSpec.shared_examples 'multi store feature flags' do |use_primary_and_secondary_stores, use_primary_store_as_default|
+  context "with feature flag :#{use_primary_and_secondary_stores} is enabled" do
+    before do
+      stub_feature_flags(use_primary_and_secondary_stores => true)
+    end
+
+    it 'multi store is enabled' do
+      expect(subject.use_primary_and_secondary_stores?).to be true
+    end
+  end
+
+  context "with feature flag :#{use_primary_and_secondary_stores} is disabled" do
+    before do
+      stub_feature_flags(use_primary_and_secondary_stores => false)
+    end
+
+    it 'multi store is disabled' do
+      expect(subject.use_primary_and_secondary_stores?).to be false
+    end
+  end
+
+  context "with feature flag :#{use_primary_store_as_default} is enabled" do
+    before do
+      stub_feature_flags(use_primary_store_as_default => true)
+    end
+
+    it 'primary store is enabled' do
+      expect(subject.use_primary_store_as_default?).to be true
+    end
+  end
+
+  context "with feature flag :#{use_primary_store_as_default} is disabled" do
+    before do
+      stub_feature_flags(use_primary_store_as_default => false)
+    end
+
+    it 'primary store is disabled' do
+      expect(subject.use_primary_store_as_default?).to be false
+    end
+  end
+end