Use Praefect instead of Gitaly in testing loop

In the test environment, we go through the trouble of spinning up Gitaly
and Praefect, only to bypass Praefect entirely and go directly to the
Gitaly socket. This renders the Praefect step useless and causes us to
miss errors in proxying.

To fix this, we now proxy all calls through Praefect and add a second
Gitaly shard.

This was discovered in
https://gitlab.com/gitlab-org/gitaly/-/issues/3379.

config/gitlab.yml.example

@@ -1386,7 +1386,7 @@ test:
     storages:
       default:
         path: tmp/tests/repositories/
-        gitaly_address: unix:tmp/tests/gitaly/gitaly.socket
+        gitaly_address: unix:tmp/tests/gitaly/praefect.socket
 
   gitaly:
     client_path: tmp/tests/gitaly

lib/gitlab/setup_helper.rb

@@ -4,10 +4,10 @@ require 'toml-rb'
 
 module Gitlab
   module SetupHelper
-    def create_configuration(dir, storage_paths, force: false)
+    def create_configuration(dir, storage_paths, force: false, options: {})
       generate_configuration(
-        configuration_toml(dir, storage_paths),
-        get_config_path(dir),
+        configuration_toml(dir, storage_paths, options),
+        get_config_path(dir, options),
         force: force
       )
     end
@@ -31,7 +31,7 @@ module Gitlab
     module Workhorse
       extend Gitlab::SetupHelper
       class << self
-        def configuration_toml(dir, _)
+        def configuration_toml(dir, _, _)
           config = { redis: { URL: redis_url } }
 
           TomlRB.dump(config)
@@ -41,8 +41,8 @@ module Gitlab
           Gitlab::Redis::SharedState.url
         end
 
-        def get_config_path(dir)
-          File.join(dir, 'config.toml')
+        def get_config_path(dir, _)
+          File.join(dir, 'config_path')
         end
 
         def compile_into(dir)
@@ -76,7 +76,7 @@ module Gitlab
         # because it uses a Unix socket.
         # For development and testing purposes, an extra storage is added to gitaly,
         # which is not known to Rails, but must be explicitly stubbed.
-        def configuration_toml(gitaly_dir, storage_paths, gitaly_ruby: true)
+        def configuration_toml(gitaly_dir, storage_paths, options, gitaly_ruby: true)
           storages = []
           address = nil
 
@@ -97,14 +97,20 @@ module Gitlab
           config = { socket_path: address.sub(/\Aunix:/, '') }
 
           if Rails.env.test?
-            storage_path = Rails.root.join('tmp', 'tests', 'second_storage').to_s
-            storages << { name: 'test_second_storage', path: storage_path }
+            socket_filename = options[:gitaly_socket] || "gitaly.socket"
 
-            config[:auth] = { token: 'secret' }
+            config = {
+              # Override the set gitaly_address since Praefect is in the loop
+              socket_path: File.join(gitaly_dir, socket_filename),
+              auth: { token: 'secret' },
               # Compared to production, tests run in constrained environments. This
               # number is meant to grow with the number of concurrent rails requests /
               # sidekiq jobs, and concurrency will be low anyway in test.
-            config[:git] = { catfile_cache_size: 5 }
+              git: { catfile_cache_size: 5 }
+            }
+
+            storage_path = Rails.root.join('tmp', 'tests', 'second_storage').to_s
+            storages << { name: 'test_second_storage', path: storage_path }
           end
 
           config[:storage] = storages
@@ -124,8 +130,9 @@ module Gitlab
 
         private
 
-        def get_config_path(dir)
-          File.join(dir, 'config.toml')
+        def get_config_path(dir, options)
+          config_filename = options[:config_filename] || 'config.toml'
+          File.join(dir, config_filename)
         end
       end
     end
@@ -133,9 +140,11 @@ module Gitlab
     module Praefect
       extend Gitlab::SetupHelper
       class << self
-        def configuration_toml(gitaly_dir, storage_paths)
+        def configuration_toml(gitaly_dir, _, _)
           nodes = [{ storage: 'default', address: "unix:#{gitaly_dir}/gitaly.socket", primary: true, token: 'secret' }]
-          storages = [{ name: 'default', node: nodes }]
+          second_storage_nodes = [{ storage: 'test_second_storage', address: "unix:#{gitaly_dir}/gitaly2.socket", primary: true, token: 'secret' }]
+
+          storages = [{ name: 'default', node: nodes }, { name: 'test_second_storage', node: second_storage_nodes }]
           failover = { enabled: false }
           config = { socket_path: "#{gitaly_dir}/praefect.socket", memory_queue_enabled: true, virtual_storage: storages, failover: failover }
           config[:token] = 'secret' if Rails.env.test?
@@ -145,7 +154,7 @@ module Gitlab
 
         private
 
-        def get_config_path(dir)
+        def get_config_path(dir, _)
           File.join(dir, 'praefect.config.toml')
         end
       end

scripts/gitaly-test-build

@@ -19,8 +19,10 @@ class GitalyTestBuild
 
     # Starting gitaly further validates its configuration
     gitaly_pid = start_gitaly
+    gitaly2_pid = start_gitaly2
     praefect_pid = start_praefect
     Process.kill('TERM', gitaly_pid)
+    Process.kill('TERM', gitaly2_pid)
     Process.kill('TERM', praefect_pid)
 
     # Make the 'gitaly' executable look newer than 'GITALY_SERVER_VERSION'.

scripts/gitaly-test-spawn

@@ -15,6 +15,7 @@ class GitalyTestSpawn
 
     # In local development this pid file is used by rspec.
     IO.write(File.expand_path('../tmp/tests/gitaly.pid', __dir__), start_gitaly)
+    IO.write(File.expand_path('../tmp/tests/gitaly2.pid', __dir__), start_gitaly2)
     IO.write(File.expand_path('../tmp/tests/praefect.pid', __dir__), start_praefect)
   end
 end

scripts/gitaly_test.rb

@@ -62,21 +62,36 @@ module GitalyTest
     case service
     when :gitaly
       File.join(tmp_tests_gitaly_dir, 'config.toml')
+    when :gitaly2
+      File.join(tmp_tests_gitaly_dir, 'gitaly2.config.toml')
     when :praefect
       File.join(tmp_tests_gitaly_dir, 'praefect.config.toml')
     end
   end
 
+  def service_binary(service)
+    case service
+    when :gitaly, :gitaly2
+      'gitaly'
+    when :praefect
+      'praefect'
+    end
+  end
+
   def start_gitaly
     start(:gitaly)
   end
 
+  def start_gitaly2
+    start(:gitaly2)
+  end
+
   def start_praefect
     start(:praefect)
   end
 
   def start(service)
-    args = ["#{tmp_tests_gitaly_dir}/#{service}"]
+    args = ["#{tmp_tests_gitaly_dir}/#{service_binary(service)}"]
     args.push("-config") if service == :praefect
     args.push(config_path(service))
     pid = spawn(env, *args, [:out, :err] => "log/#{service}-test.log")

spec/support/helpers/test_env.rb

@@ -168,6 +168,11 @@ module TestEnv
       version: Gitlab::GitalyClient.expected_server_version,
       task: "gitlab:gitaly:install[#{install_gitaly_args}]") do
         Gitlab::SetupHelper::Gitaly.create_configuration(gitaly_dir, { 'default' => repos_path }, force: true)
+        Gitlab::SetupHelper::Gitaly.create_configuration(
+          gitaly_dir,
+          { 'default' => repos_path }, force: true,
+          options: { gitaly_socket: "gitaly2.socket", config_filename: "gitaly2.config.toml" }
+        )
         Gitlab::SetupHelper::Praefect.create_configuration(gitaly_dir, { 'praefect' => repos_path }, force: true)
       end
 
@@ -283,7 +288,7 @@ module TestEnv
     host = "[#{host}]" if host.include?(':')
     listen_addr = [host, port].join(':')
 
-    config_path = Gitlab::SetupHelper::Workhorse.get_config_path(workhorse_dir)
+    config_path = Gitlab::SetupHelper::Workhorse.get_config_path(workhorse_dir, {})
 
     # This should be set up in setup_workhorse, but since
     # component_needs_update? only checks that versions are consistent,