Merge branch 'cleanup-use-model-load-balancing-feature-flag' into 'master'

Remove use_model_load_balancing feature flag and GITLAB_USE_MODEL_LOAD_BALANCING env var

See merge request gitlab-org/gitlab!83162

.gitlab/ci/rails.gitlab-ci.yml

@@ -24,8 +24,6 @@
 
 .single-db-rspec:
   extends: .single-db
-  variables:
-    GITLAB_USE_MODEL_LOAD_BALANCING: "false"
 
 .rspec-base:
   extends:
@@ -387,8 +385,6 @@ db:migrate-from-previous-major-version:
     SETUP_DB: "false"
     PROJECT_TO_CHECKOUT: "gitlab-foss"
     TAG_TO_CHECKOUT: "v13.12.9"
-    # FIXME: make this job work with `GITLAB_USE_MODEL_LOAD_BALANCING: true`, see https://gitlab.com/gitlab-org/gitlab/-/issues/355573
-    GITLAB_USE_MODEL_LOAD_BALANCING: "false"
   before_script:
     - !reference [.default-before_script, before_script]
     - '[[ -d "ee/" ]] || export PROJECT_TO_CHECKOUT="gitlab"'

config/feature_flags/development/use_model_load_balancing.yml

----
-name: use_model_load_balancing
-introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/73631
-rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/344797
-milestone: '14.5'
-type: development
-group: group::sharding
-default_enabled: false

lib/gitlab/database/load_balancing/connection_proxy.rb

@@ -13,13 +13,6 @@ module Gitlab
         WriteInsideReadOnlyTransactionError = Class.new(StandardError)
         READ_ONLY_TRANSACTION_KEY = :load_balacing_read_only_transaction
 
-        # The load balancer returned by connection might be different
-        # between `model.connection.load_balancer` vs `model.load_balancer`
-        #
-        # The used `model.connection` is dependent on `use_model_load_balancing`.
-        # See more in: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/73949.
-        #
-        # Always use `model.load_balancer` or `model.sticking`.
         attr_reader :load_balancer
 
         # These methods perform writes after which we need to stick to the

lib/gitlab/database/load_balancing/setup.rb

@@ -17,7 +17,6 @@ module Gitlab
           configure_connection
           setup_connection_proxy
           setup_service_discovery
-          setup_feature_flag_to_model_load_balancing
         end
 
         def configure_connection
@@ -45,21 +44,6 @@ module Gitlab
           setup_class_attribute(:sticking, Sticking.new(load_balancer))
         end
 
-        # TODO: This is temporary code to gradually redirect traffic to use
-        # a dedicated DB replicas, or DB primaries (depending on configuration)
-        # This implements a sticky behavior for the current request if enabled.
-        #
-        # This is needed for Phase 3 and Phase 4 of application rollout
-        # https://gitlab.com/groups/gitlab-org/-/epics/6160#progress
-        #
-        # If `GITLAB_USE_MODEL_LOAD_BALANCING` is set, its value is preferred
-        # Otherwise, a `use_model_load_balancing` FF value is used
-        def setup_feature_flag_to_model_load_balancing
-          return if active_record_base?
-
-          @model.singleton_class.prepend(ModelLoadBalancingFeatureFlagMixin)
-        end
-
         def setup_service_discovery
           return unless configuration.service_discovery_enabled?
 
@@ -84,31 +68,6 @@ module Gitlab
         def active_record_base?
           @model == ActiveRecord::Base
         end
-
-        module ModelLoadBalancingFeatureFlagMixin
-          extend ActiveSupport::Concern
-
-          def use_model_load_balancing?
-            # Cache environment variable and return env variable first if defined
-            default_use_model_load_balancing_env = Gitlab.dev_or_test_env? || nil
-            use_model_load_balancing_env = Gitlab::Utils.to_boolean(ENV.fetch('GITLAB_USE_MODEL_LOAD_BALANCING', default_use_model_load_balancing_env))
-
-            unless use_model_load_balancing_env.nil?
-              return use_model_load_balancing_env
-            end
-
-            # Check a feature flag using RequestStore (if active)
-            return false unless Gitlab::SafeRequestStore.active?
-
-            Gitlab::SafeRequestStore.fetch(:use_model_load_balancing) do
-              Feature.enabled?(:use_model_load_balancing, default_enabled: :yaml)
-            end
-          end
-
-          def connection
-            use_model_load_balancing? ? super : ApplicationRecord.connection
-          end
-        end
       end
     end
   end

spec/lib/gitlab/database/load_balancing/setup_spec.rb

@@ -10,7 +10,6 @@ RSpec.describe Gitlab::Database::LoadBalancing::Setup do
       expect(setup).to receive(:configure_connection)
       expect(setup).to receive(:setup_connection_proxy)
       expect(setup).to receive(:setup_service_discovery)
-      expect(setup).to receive(:setup_feature_flag_to_model_load_balancing)
 
       setup.setup
     end
@@ -120,120 +119,46 @@ RSpec.describe Gitlab::Database::LoadBalancing::Setup do
     end
   end
 
-  describe '#setup_feature_flag_to_model_load_balancing', :reestablished_active_record_base do
+  context 'uses correct base models', :reestablished_active_record_base do
     using RSpec::Parameterized::TableSyntax
 
     where do
       {
-        "with model LB enabled it picks a dedicated CI connection" => {
-          env_GITLAB_USE_MODEL_LOAD_BALANCING: 'true',
+        "it picks a dedicated CI connection" => {
           env_GITLAB_LOAD_BALANCING_REUSE_PRIMARY_ci: nil,
           request_store_active: false,
-          ff_use_model_load_balancing: nil,
           ff_force_no_sharing_primary_model: false,
           expectations: {
             main: { read: 'main_replica', write: 'main' },
             ci: { read: 'ci_replica', write: 'ci' }
           }
         },
-        "with model LB enabled and re-use of primary connection it uses CI connection for reads" => {
-          env_GITLAB_USE_MODEL_LOAD_BALANCING: 'true',
+        "with re-use of primary connection it uses CI connection for reads" => {
           env_GITLAB_LOAD_BALANCING_REUSE_PRIMARY_ci: 'main',
           request_store_active: false,
-          ff_use_model_load_balancing: nil,
           ff_force_no_sharing_primary_model: false,
           expectations: {
             main: { read: 'main_replica', write: 'main' },
             ci: { read: 'ci_replica', write: 'main' }
           }
         },
-        "with model LB disabled it fallbacks to use main" => {
-          env_GITLAB_USE_MODEL_LOAD_BALANCING: 'false',
-          env_GITLAB_LOAD_BALANCING_REUSE_PRIMARY_ci: nil,
-          request_store_active: false,
-          ff_use_model_load_balancing: nil,
-          ff_force_no_sharing_primary_model: false,
-          expectations: {
-            main: { read: 'main_replica', write: 'main' },
-            ci: { read: 'main_replica', write: 'main' }
-          }
-        },
-        "with model LB disabled, but re-use configured it fallbacks to use main" => {
-          env_GITLAB_USE_MODEL_LOAD_BALANCING: 'false',
+        "with re-use and FF force_no_sharing_primary_model enabled with RequestStore it sticks FF and uses CI connection for reads and writes" => {
           env_GITLAB_LOAD_BALANCING_REUSE_PRIMARY_ci: 'main',
-          request_store_active: false,
-          ff_use_model_load_balancing: nil,
-          ff_force_no_sharing_primary_model: false,
-          expectations: {
-            main: { read: 'main_replica', write: 'main' },
-            ci: { read: 'main_replica', write: 'main' }
-          }
-        },
-        "with FF use_model_load_balancing disabled without RequestStore it uses main" => {
-          env_GITLAB_USE_MODEL_LOAD_BALANCING: nil,
-          env_GITLAB_LOAD_BALANCING_REUSE_PRIMARY_ci: nil,
-          request_store_active: false,
-          ff_use_model_load_balancing: false,
-          ff_force_no_sharing_primary_model: false,
-          expectations: {
-            main: { read: 'main_replica', write: 'main' },
-            ci: { read: 'main_replica', write: 'main' }
-          }
-        },
-        "with FF use_model_load_balancing enabled without RequestStore sticking of FF does not work, so it fallbacks to use main" => {
-          env_GITLAB_USE_MODEL_LOAD_BALANCING: nil,
-          env_GITLAB_LOAD_BALANCING_REUSE_PRIMARY_ci: nil,
-          request_store_active: false,
-          ff_use_model_load_balancing: true,
-          ff_force_no_sharing_primary_model: false,
-          expectations: {
-            main: { read: 'main_replica', write: 'main' },
-            ci: { read: 'main_replica', write: 'main' }
-          }
-        },
-        "with FF use_model_load_balancing disabled with RequestStore it uses main" => {
-          env_GITLAB_USE_MODEL_LOAD_BALANCING: nil,
-          env_GITLAB_LOAD_BALANCING_REUSE_PRIMARY_ci: nil,
-          request_store_active: true,
-          ff_use_model_load_balancing: false,
-          ff_force_no_sharing_primary_model: false,
-          expectations: {
-            main: { read: 'main_replica', write: 'main' },
-            ci: { read: 'main_replica', write: 'main' }
-          }
-        },
-        "with FF use_model_load_balancing enabled with RequestStore it sticks FF and uses CI connection" => {
-          env_GITLAB_USE_MODEL_LOAD_BALANCING: nil,
-          env_GITLAB_LOAD_BALANCING_REUSE_PRIMARY_ci: nil,
           request_store_active: true,
-          ff_use_model_load_balancing: true,
-          ff_force_no_sharing_primary_model: false,
+          ff_force_no_sharing_primary_model: true,
           expectations: {
             main: { read: 'main_replica', write: 'main' },
             ci: { read: 'ci_replica', write: 'ci' }
           }
         },
-        "with re-use and ff_use_model_load_balancing enabled and FF force_no_sharing_primary_model disabled with RequestStore it sticks FF and uses CI connection for reads" => {
-          env_GITLAB_USE_MODEL_LOAD_BALANCING: nil,
+        "with re-use and FF force_no_sharing_primary_model enabled without RequestStore it doesn't use FF and uses CI connection for reads only" => {
           env_GITLAB_LOAD_BALANCING_REUSE_PRIMARY_ci: 'main',
           request_store_active: true,
-          ff_use_model_load_balancing: true,
           ff_force_no_sharing_primary_model: false,
           expectations: {
             main: { read: 'main_replica', write: 'main' },
             ci: { read: 'ci_replica', write: 'main' }
           }
-        },
-        "with re-use and ff_use_model_load_balancing enabled and FF force_no_sharing_primary_model enabled with RequestStore it sticks FF and uses CI connection for reads" => {
-          env_GITLAB_USE_MODEL_LOAD_BALANCING: nil,
-          env_GITLAB_LOAD_BALANCING_REUSE_PRIMARY_ci: 'main',
-          request_store_active: true,
-          ff_use_model_load_balancing: true,
-          ff_force_no_sharing_primary_model: true,
-          expectations: {
-            main: { read: 'main_replica', write: 'main' },
-            ci: { read: 'ci_replica', write: 'ci' }
-          }
         }
       }
     end
@@ -285,9 +210,7 @@ RSpec.describe Gitlab::Database::LoadBalancing::Setup do
           end
         end
 
-        stub_env('GITLAB_USE_MODEL_LOAD_BALANCING', env_GITLAB_USE_MODEL_LOAD_BALANCING)
         stub_env('GITLAB_LOAD_BALANCING_REUSE_PRIMARY_ci', env_GITLAB_LOAD_BALANCING_REUSE_PRIMARY_ci)
-        stub_feature_flags(use_model_load_balancing: ff_use_model_load_balancing)
 
         # Make load balancer to force init with a dedicated replicas connections
         models.each do |_, model|

spec/lib/gitlab/database_spec.rb

@@ -251,9 +251,6 @@ RSpec.describe Gitlab::Database do
     end
 
     it 'does return a valid schema depending on a base model used', :request_store do
-      # This is currently required as otherwise the `Ci::Build.connection` == `Project.connection`
-      # ENV due to lib/gitlab/database/load_balancing/setup.rb:93
-      stub_env('GITLAB_USE_MODEL_LOAD_BALANCING', '1')
       # FF due to lib/gitlab/database/load_balancing/configuration.rb:92
       stub_feature_flags(force_no_sharing_primary_model: true)