Merge branch 'fix-gitaly-bundle-poisoning' into 'master'

Fix gitaly-ruby bundle poisoning in CI

Closes gitaly#1184 and gitaly#1169

See merge request gitlab-org/gitlab-ce!18853

.gitlab-ci.yml

@@ -10,6 +10,7 @@ image: "dev.gitlab.org:5005/gitlab/gitlab-build-images:ruby-2.3.7-golang-1.9-git
   paths:
     - vendor/ruby
     - .yarn-cache/
+    - vendor/gitaly-ruby
 
 .push-cache: &push-cache
   cache:
@@ -438,6 +439,7 @@ setup-test-env:
     paths:
       - tmp/tests
       - config/secrets.yml
+      - vendor/gitaly-ruby
 
 rspec-pg 0 28: *rspec-metadata-pg
 rspec-pg 1 28: *rspec-metadata-pg

scripts/gitaly-test-build

@@ -2,28 +2,29 @@
 
 require 'fileutils'
 
+require_relative 'gitaly_test'
+
 # This script assumes tmp/tests/gitaly already contains the correct
 # Gitaly version. We just have to compile it and run its 'bundle
-# install'. We have this separate script for that because weird things
-# were happening in CI when we have a 'bundle exec' process that later
-# called 'bundle install' using a different Gemfile, as happens with
-# gitlab-ce and gitaly.
+# install'. We have this separate script for that to avoid bundle
+# poisoning in CI. This script should only be run in CI.
+class GitalyTestBuild
+  include GitalyTest
 
-tmp_tests_gitaly_dir = File.expand_path('../tmp/tests/gitaly', __dir__)
+  def run
+    abort 'gitaly build failed' unless system(env, 'make', chdir: tmp_tests_gitaly_dir)
 
-# Use the top-level bundle vendor folder so that we don't reinstall gems twice
-bundle_vendor_path = File.expand_path('../vendor', __dir__)
+    check_gitaly_config!
 
-env = {
-  # This ensure the `clean` config set in `scripts/prepare_build.sh` isn't taken into account
-  'BUNDLE_IGNORE_CONFIG' => 'true',
-  'BUNDLE_GEMFILE' => File.join(tmp_tests_gitaly_dir, 'ruby', 'Gemfile'),
-  'BUNDLE_FLAGS' => "--jobs=4 --path=#{bundle_vendor_path} --retry=3"
-}
+    # Starting gitaly further validates its configuration
+    pid = start_gitaly
+    Process.kill('TERM', pid)
 
-abort 'gitaly build failed' unless system(env, 'make', chdir: tmp_tests_gitaly_dir)
+    # Make the 'gitaly' executable look newer than 'GITALY_SERVER_VERSION'.
+    # Without this a gitaly executable created in the setup-test-env job
+    # will look stale compared to GITALY_SERVER_VERSION.
+    FileUtils.touch(File.join(tmp_tests_gitaly_dir, 'gitaly'), mtime: Time.now + (1 << 24))
+  end
+end
 
-# Make the 'gitaly' executable look newer than 'GITALY_SERVER_VERSION'.
-# Without this a gitaly executable created in the setup-test-env job
-# will look stale compared to GITALY_SERVER_VERSION.
-FileUtils.touch(File.join(tmp_tests_gitaly_dir, 'gitaly'), mtime: Time.now + (1 << 24))
+GitalyTestBuild.new.run

scripts/gitaly-test-spawn

 #!/usr/bin/env ruby
 
-gitaly_dir = 'tmp/tests/gitaly'
-env = { 'HOME' => File.expand_path('tmp/tests'),
-        'GEM_PATH' => Gem.path.join(':') }
-args = %W[#{gitaly_dir}/gitaly #{gitaly_dir}/config.toml]
+# This script is used both in CI and in local development 'rspec' runs.
 
-# Print the PID of the spawned process
-puts spawn(env, *args, [:out, :err] => 'log/gitaly-test.log')
+require_relative 'gitaly_test'
+
+class GitalyTestSpawn
+  include GitalyTest
+
+  def run
+    check_gitaly_config!
+
+    # # Uncomment line below to see all gitaly logs merged into CI trace
+    # spawn('sleep 1; tail -f log/gitaly-test.log')
+
+    pid = start_gitaly
+
+    # In local development this pid file is used by rspec.
+    IO.write(File.expand_path('../tmp/tests/gitaly.pid', __dir__), pid)
+  end
+end
+
+GitalyTestSpawn.new.run

scripts/gitaly_test.rb

+# This file contains environment settings for gitaly when it's running
+# as part of the gitlab-ce/ee test suite.
+#
+# Please be careful when modifying this file. Your changes must work
+# both for local development rspec runs, and in CI.
+
+require 'socket'
+
+module GitalyTest
+  def tmp_tests_gitaly_dir
+    File.expand_path('../tmp/tests/gitaly', __dir__)
+  end
+
+  def gemfile
+    File.join(tmp_tests_gitaly_dir, 'ruby', 'Gemfile')
+  end
+
+  def env
+    env_hash = {
+      'HOME' => File.expand_path('tmp/tests'),
+      'GEM_PATH' => Gem.path.join(':'),
+      'BUNDLE_APP_CONFIG' => File.join(File.dirname(gemfile), '.bundle/config'),
+      'BUNDLE_FLAGS' => "--jobs=4 --retry=3",
+      'BUNDLE_INSTALL_FLAGS' => nil,
+      'BUNDLE_GEMFILE' => gemfile,
+      'RUBYOPT' => nil
+    }
+
+    if ENV['CI']
+      bundle_path = File.expand_path('../vendor/gitaly-ruby', __dir__)
+      env_hash['BUNDLE_FLAGS'] << " --path=#{bundle_path}"
+    end
+
+    env_hash
+  end
+
+  def config_path
+    File.join(tmp_tests_gitaly_dir, 'config.toml')
+  end
+
+  def start_gitaly
+    args = %W[#{tmp_tests_gitaly_dir}/gitaly #{config_path}]
+    pid = spawn(env, *args, [:out, :err] => 'log/gitaly-test.log')
+
+    begin
+      try_connect!
+    rescue
+      Process.kill('TERM', pid)
+      raise
+    end
+
+    pid
+  end
+
+  def check_gitaly_config!
+    puts 'Checking gitaly-ruby bundle...'
+    abort 'bundle check failed' unless system(env, 'bundle', 'check', chdir: File.dirname(gemfile))
+  end
+
+  def read_socket_path
+    # This code needs to work in an environment where we cannot use bundler,
+    # so we cannot easily use the toml-rb gem. This ad-hoc parser should be
+    # good enough.
+    config_text = IO.read(config_path)
+
+    config_text.lines.each do |line|
+      match_data = line.match(/^\s*socket_path\s*=\s*"([^"]*)"$/)
+
+      return match_data[1] if match_data
+    end
+
+    raise "failed to find socket_path in #{config_path}"
+  end
+
+  def try_connect!
+    print "Trying to connect to gitaly: "
+    timeout = 20
+    delay = 0.1
+    socket = read_socket_path
+
+    Integer(timeout / delay).times do
+      begin
+        UNIXSocket.new(socket)
+        puts ' OK'
+
+        return
+      rescue Errno::ENOENT, Errno::ECONNREFUSED
+        print '.'
+        sleep delay
+      end
+    end
+
+    puts ' FAILED'
+
+    raise "could not connect to #{socket}"
+  end
+end

spec/support/helpers/test_env.rb

@@ -159,7 +159,11 @@ module TestEnv
     end
 
     spawn_script = Rails.root.join('scripts/gitaly-test-spawn').to_s
-    @gitaly_pid = Bundler.with_original_env { IO.popen([spawn_script], &:read).to_i }
+    Bundler.with_original_env do
+      raise "gitaly spawn failed" unless system(spawn_script)
+    end
+    @gitaly_pid = Integer(File.read('tmp/tests/gitaly.pid'))
+
     Kernel.at_exit { stop_gitaly }
 
     wait_gitaly