Speed up operations performed by gitlab-shell

lib/gitlab/shell.rb

@@ -2,6 +2,8 @@ require 'securerandom'
 
 module Gitlab
   class Shell
+    GITLAB_SHELL_ENV_VARS = %w(GIT_TERMINAL_PROMPT).freeze
+
     Error = Class.new(StandardError)
 
     KeyAdder = Struct.new(:io) do
@@ -67,8 +69,8 @@ module Gitlab
     #   add_repository("/path/to/storage", "gitlab/gitlab-ci")
     #
     def add_repository(storage, name)
-      Gitlab::Utils.system_silent([gitlab_shell_projects_path,
-                                   'add-project', storage, "#{name}.git"])
+      gitlab_shell_fast_execute([gitlab_shell_projects_path,
+                                 'add-project', storage, "#{name}.git"])
     end
 
     # Import repository
@@ -82,10 +84,9 @@ module Gitlab
     def import_repository(storage, name, url)
       # Timeout should be less than 900 ideally, to prevent the memory killer
       # to silently kill the process without knowing we are timing out here.
-      output, status = Popen.popen([gitlab_shell_projects_path, 'import-project',
-                                    storage, "#{name}.git", url, "#{Gitlab.config.gitlab_shell.git_timeout}"])
-      raise Error, output unless status.zero?
-      true
+      cmd = [gitlab_shell_projects_path, 'import-project',
+             storage, "#{name}.git", url, "#{Gitlab.config.gitlab_shell.git_timeout}"]
+      gitlab_shell_fast_execute_raise_error(cmd)
     end
 
     # Fetch remote for repository
@@ -103,9 +104,7 @@ module Gitlab
       args << '--force' if forced
       args << '--no-tags' if no_tags
 
-      output, status = Popen.popen(args)
-      raise Error, output unless status.zero?
-      true
+      gitlab_shell_fast_execute_raise_error(args)
     end
 
     # Move repository
@@ -117,8 +116,8 @@ module Gitlab
     #   mv_repository("/path/to/storage", "gitlab/gitlab-ci", "randx/gitlab-ci-new")
     #
     def mv_repository(storage, path, new_path)
-      Gitlab::Utils.system_silent([gitlab_shell_projects_path, 'mv-project',
-                                   storage, "#{path}.git", "#{new_path}.git"])
+      gitlab_shell_fast_execute([gitlab_shell_projects_path, 'mv-project',
+                                 storage, "#{path}.git", "#{new_path}.git"])
     end
 
     # Fork repository to new namespace
@@ -131,9 +130,9 @@ module Gitlab
     #  fork_repository("/path/to/forked_from/storage", "gitlab/gitlab-ci", "/path/to/forked_to/storage", "randx")
     #
     def fork_repository(forked_from_storage, path, forked_to_storage, fork_namespace)
-      Gitlab::Utils.system_silent([gitlab_shell_projects_path, 'fork-project',
-                                   forked_from_storage, "#{path}.git", forked_to_storage,
-                                   fork_namespace])
+      gitlab_shell_fast_execute([gitlab_shell_projects_path, 'fork-project',
+                                 forked_from_storage, "#{path}.git", forked_to_storage,
+                                 fork_namespace])
     end
 
     # Remove repository from file system
@@ -145,8 +144,8 @@ module Gitlab
     #   remove_repository("/path/to/storage", "gitlab/gitlab-ci")
     #
     def remove_repository(storage, name)
-      Gitlab::Utils.system_silent([gitlab_shell_projects_path,
-                                   'rm-project', storage, "#{name}.git"])
+      gitlab_shell_fast_execute([gitlab_shell_projects_path,
+                                 'rm-project', storage, "#{name}.git"])
     end
 
     # Add new key to gitlab-shell
@@ -155,8 +154,8 @@ module Gitlab
     #   add_key("key-42", "sha-rsa ...")
     #
     def add_key(key_id, key_content)
-      Gitlab::Utils.system_silent([gitlab_shell_keys_path,
-                                   'add-key', key_id, self.class.strip_key(key_content)])
+      gitlab_shell_fast_execute([gitlab_shell_keys_path,
+                                 'add-key', key_id, self.class.strip_key(key_content)])
     end
 
     # Batch-add keys to authorized_keys
@@ -175,8 +174,10 @@ module Gitlab
     #   remove_key("key-342", "sha-rsa ...")
     #
     def remove_key(key_id, key_content)
-      Gitlab::Utils.system_silent([gitlab_shell_keys_path,
-                                   'rm-key', key_id, key_content])
+      args = [gitlab_shell_keys_path, 'rm-key', key_id]
+      args << key_content if key_content
+
+      gitlab_shell_fast_execute(args)
     end
 
     # Remove all ssh keys from gitlab shell
@@ -185,7 +186,7 @@ module Gitlab
     #   remove_all_keys
     #
     def remove_all_keys
-      Gitlab::Utils.system_silent([gitlab_shell_keys_path, 'clear'])
+      gitlab_shell_fast_execute([gitlab_shell_keys_path, 'clear'])
     end
 
     # Add empty directory for storing repositories
@@ -267,5 +268,31 @@ module Gitlab
     def gitlab_shell_keys_path
       File.join(gitlab_shell_path, 'bin', 'gitlab-keys')
     end
+
+    private
+
+    def gitlab_shell_fast_execute(cmd)
+      output, status = gitlab_shell_fast_execute_helper(cmd)
+
+      return true if status.zero?
+
+      Rails.logger.error("gitlab-shell failed with error #{status}: #{output}")
+      false
+    end
+
+    def gitlab_shell_fast_execute_raise_error(cmd)
+      output, status = gitlab_shell_fast_execute_helper(cmd)
+
+      raise Error, output unless status.zero?
+      true
+    end
+
+    def gitlab_shell_fast_execute_helper(cmd)
+      vars = ENV.to_h.slice(*GITLAB_SHELL_ENV_VARS)
+
+      # Don't pass along the entire parent environment to prevent gitlab-shell
+      # from wasting I/O by searching through GEM_PATH
+      Bundler.with_original_env { Popen.popen(cmd, nil, vars) }
+    end
   end
 end

spec/lib/gitlab/popen_spec.rb

@@ -32,6 +32,17 @@ describe 'Gitlab::Popen', lib: true, no_db: true do
     end
   end
 
+  context 'with custom options' do
+    let(:vars) { { 'foobar' => 123, 'PWD' => path } }
+    let(:options) { { chdir: path } }
+
+    it 'calls popen3 with the provided environment variables' do
+      expect(Open3).to receive(:popen3).with(vars, 'ls', options)
+
+      @output, @status = @klass.new.popen(%w(ls), path, { 'foobar' => 123 })
+    end
+  end
+
   context 'without a directory argument' do
     before do
       @output, @status = @klass.new.popen(%w(ls))
@@ -45,7 +56,7 @@ describe 'Gitlab::Popen', lib: true, no_db: true do
     before do
       @output, @status = @klass.new.popen(%w[cat]) { |stdin| stdin.write 'hello' }
     end
-  
+
     it { expect(@status).to be_zero }
     it { expect(@output).to eq('hello') }
   end

spec/lib/gitlab/shell_spec.rb

@@ -4,6 +4,7 @@ require 'stringio'
 describe Gitlab::Shell, lib: true do
   let(:project) { double('Project', id: 7, path: 'diaspora') }
   let(:gitlab_shell) { Gitlab::Shell.new }
+  let(:popen_vars) { { 'GIT_TERMINAL_PROMPT' => ENV['GIT_TERMINAL_PROMPT'] } }
 
   before do
     allow(Project).to receive(:find).and_return(project)
@@ -50,7 +51,7 @@ describe Gitlab::Shell, lib: true do
   describe '#add_key' do
     it 'removes trailing garbage' do
       allow(gitlab_shell).to receive(:gitlab_shell_keys_path).and_return(:gitlab_shell_keys_path)
-      expect(Gitlab::Utils).to receive(:system_silent).with(
+      expect(gitlab_shell).to receive(:gitlab_shell_fast_execute).with(
         [:gitlab_shell_keys_path, 'add-key', 'key-123', 'ssh-rsa foobar']
       )
 
@@ -100,17 +101,91 @@ describe Gitlab::Shell, lib: true do
       allow(Gitlab.config.gitlab_shell).to receive(:git_timeout).and_return(800)
     end
 
+    describe '#add_repository' do
+      it 'returns true when the command succeeds' do
+        expect(Gitlab::Popen).to receive(:popen)
+          .with([projects_path, 'add-project', 'current/storage', 'project/path.git'],
+                nil, popen_vars).and_return([nil, 0])
+
+        expect(gitlab_shell.add_repository('current/storage', 'project/path')).to be true
+      end
+
+      it 'returns false when the command fails' do
+        expect(Gitlab::Popen).to receive(:popen)
+          .with([projects_path, 'add-project', 'current/storage', 'project/path.git'],
+                nil, popen_vars).and_return(["error", 1])
+
+        expect(gitlab_shell.add_repository('current/storage', 'project/path')).to be false
+      end
+    end
+
+    describe '#remove_repository' do
+      it 'returns true when the command succeeds' do
+        expect(Gitlab::Popen).to receive(:popen)
+          .with([projects_path, 'rm-project', 'current/storage', 'project/path.git'],
+                nil, popen_vars).and_return([nil, 0])
+
+        expect(gitlab_shell.remove_repository('current/storage', 'project/path')).to be true
+      end
+
+      it 'returns false when the command fails' do
+        expect(Gitlab::Popen).to receive(:popen)
+          .with([projects_path, 'rm-project', 'current/storage', 'project/path.git'],
+                nil, popen_vars).and_return(["error", 1])
+
+        expect(gitlab_shell.remove_repository('current/storage', 'project/path')).to be false
+      end
+    end
+
+    describe '#mv_repository' do
+      it 'returns true when the command succeeds' do
+        expect(Gitlab::Popen).to receive(:popen)
+          .with([projects_path, 'mv-project', 'current/storage', 'project/path.git', 'project/newpath.git'],
+                nil, popen_vars).and_return([nil, 0])
+
+        expect(gitlab_shell.mv_repository('current/storage', 'project/path', 'project/newpath')).to be true
+      end
+
+      it 'returns false when the command fails' do
+        expect(Gitlab::Popen).to receive(:popen)
+          .with([projects_path, 'mv-project', 'current/storage', 'project/path.git', 'project/newpath.git'],
+                nil, popen_vars).and_return(["error", 1])
+
+        expect(gitlab_shell.mv_repository('current/storage', 'project/path', 'project/newpath')).to be false
+      end
+    end
+
+    describe '#fork_repository' do
+      it 'returns true when the command succeeds' do
+        expect(Gitlab::Popen).to receive(:popen)
+          .with([projects_path, 'fork-project', 'current/storage', 'project/path.git', 'new/storage', 'new-namespace'],
+                nil, popen_vars).and_return([nil, 0])
+
+        expect(gitlab_shell.fork_repository('current/storage', 'project/path', 'new/storage', 'new-namespace')).to be true
+      end
+
+      it 'return false when the command fails' do
+        expect(Gitlab::Popen).to receive(:popen)
+          .with([projects_path, 'fork-project', 'current/storage', 'project/path.git', 'new/storage', 'new-namespace'],
+                nil, popen_vars).and_return(["error", 1])
+
+        expect(gitlab_shell.fork_repository('current/storage', 'project/path', 'new/storage', 'new-namespace')).to be false
+      end
+    end
+
     describe '#fetch_remote' do
       it 'returns true when the command succeeds' do
         expect(Gitlab::Popen).to receive(:popen)
-          .with([projects_path, 'fetch-remote', 'current/storage', 'project/path.git', 'new/storage', '800']).and_return([nil, 0])
+          .with([projects_path, 'fetch-remote', 'current/storage', 'project/path.git', 'new/storage', '800'],
+                nil, popen_vars).and_return([nil, 0])
 
         expect(gitlab_shell.fetch_remote('current/storage', 'project/path', 'new/storage')).to be true
       end
 
       it 'raises an exception when the command fails' do
         expect(Gitlab::Popen).to receive(:popen)
-        .with([projects_path, 'fetch-remote', 'current/storage', 'project/path.git', 'new/storage', '800']).and_return(["error", 1])
+        .with([projects_path, 'fetch-remote', 'current/storage', 'project/path.git', 'new/storage', '800'],
+              nil, popen_vars).and_return(["error", 1])
 
         expect { gitlab_shell.fetch_remote('current/storage', 'project/path', 'new/storage') }.to raise_error(Gitlab::Shell::Error, "error")
       end
@@ -119,14 +194,16 @@ describe Gitlab::Shell, lib: true do
     describe '#import_repository' do
       it 'returns true when the command succeeds' do
         expect(Gitlab::Popen).to receive(:popen)
-          .with([projects_path, 'import-project', 'current/storage', 'project/path.git', 'https://gitlab.com/gitlab-org/gitlab-ce.git', "800"]).and_return([nil, 0])
+          .with([projects_path, 'import-project', 'current/storage', 'project/path.git', 'https://gitlab.com/gitlab-org/gitlab-ce.git', "800"],
+                nil, popen_vars).and_return([nil, 0])
 
         expect(gitlab_shell.import_repository('current/storage', 'project/path', 'https://gitlab.com/gitlab-org/gitlab-ce.git')).to be true
       end
 
       it 'raises an exception when the command fails' do
         expect(Gitlab::Popen).to receive(:popen)
-        .with([projects_path, 'import-project', 'current/storage', 'project/path.git', 'https://gitlab.com/gitlab-org/gitlab-ce.git', "800"]).and_return(["error", 1])
+        .with([projects_path, 'import-project', 'current/storage', 'project/path.git', 'https://gitlab.com/gitlab-org/gitlab-ce.git', "800"],
+              nil, popen_vars).and_return(["error", 1])
 
         expect { gitlab_shell.import_repository('current/storage', 'project/path', 'https://gitlab.com/gitlab-org/gitlab-ce.git') }.to raise_error(Gitlab::Shell::Error, "error")
       end