From b77176d11a7a31acd38b05aa39afc9ebed5a3915 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kim=20=22BKC=22=20Carlb=C3=A4cker?= Date: Thu, 10 Aug 2017 16:08:48 +0200 Subject: [PATCH] Migrate Repository.FetchRemote to Gitaly - `Gitlab::Shell.fetch_remote` now takes a `Gitlab::Git::Repository` instead --- GITALY_SERVER_VERSION | 2 +- app/models/repository.rb | 2 +- lib/gitlab/git/repository.rb | 3 + .../gitaly_client/repository_service.rb | 16 +++++ lib/gitlab/shell.rb | 55 ++++++++++------ spec/lib/gitlab/shell_spec.rb | 64 ++++++++++++++----- 6 files changed, 107 insertions(+), 35 deletions(-) diff --git a/GITALY_SERVER_VERSION b/GITALY_SERVER_VERSION index be386c9ede3..85e60ed180c 100644 --- a/GITALY_SERVER_VERSION +++ b/GITALY_SERVER_VERSION @@ -1 +1 @@ -0.33.0 +0.34.0 diff --git a/app/models/repository.rb b/app/models/repository.rb index 9fb2e2aa306..9368207b0fb 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -1027,7 +1027,7 @@ class Repository end def fetch_remote(remote, forced: false, no_tags: false) - gitlab_shell.fetch_remote(repository_storage_path, disk_path, remote, forced: forced, no_tags: no_tags) + gitlab_shell.fetch_remote(raw_repository, remote, forced: forced, no_tags: no_tags) end def fetch_ref(source_path, source_ref, target_ref) diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb index b835dec24eb..ec923c990ae 100644 --- a/lib/gitlab/git/repository.rb +++ b/lib/gitlab/git/repository.rb @@ -46,6 +46,9 @@ module Gitlab # Directory name of repo attr_reader :name + # Relative path of repo + attr_reader :relative_path + # Rugged repo object attr_reader :rugged diff --git a/lib/gitlab/gitaly_client/repository_service.rb b/lib/gitlab/gitaly_client/repository_service.rb index a74a6dc6e78..177a1284f38 100644 --- a/lib/gitlab/gitaly_client/repository_service.rb +++ b/lib/gitlab/gitaly_client/repository_service.rb @@ -37,6 +37,22 @@ module Gitlab request = Gitaly::ApplyGitattributesRequest.new(repository: @gitaly_repo, revision: revision) GitalyClient.call(@storage, :repository_service, :apply_gitattributes, request) end + + def fetch_remote(remote, ssh_auth: nil, forced: false, no_tags: false) + request = Gitaly::FetchRemoteRequest.new(repository: @gitaly_repo, remote: remote, force: forced, no_tags: no_tags) + + if ssh_auth&.ssh_import? + if ssh_auth.ssh_key_auth? && ssh_auth.ssh_private_key.present? + request.ssh_key = ssh_auth.ssh_private_key + end + + if ssh_auth.ssh_known_hosts.present? + request.known_hosts = ssh_auth.ssh_known_hosts + end + end + + GitalyClient.call(@storage, :repository_service, :fetch_remote, request) + end end end end diff --git a/lib/gitlab/shell.rb b/lib/gitlab/shell.rb index 280a9abf03e..81ecdf43ef9 100644 --- a/lib/gitlab/shell.rb +++ b/lib/gitlab/shell.rb @@ -98,33 +98,24 @@ module Gitlab # Fetch remote for repository # - # name - project path with namespace + # repository - an instance of Git::Repository # remote - remote name # forced - should we use --force flag? # no_tags - should we use --no-tags flag? # # Ex. - # fetch_remote("gitlab/gitlab-ci", "upstream") + # fetch_remote(my_repo, "upstream") # # Gitaly migration: https://gitlab.com/gitlab-org/gitaly/issues/387 - def fetch_remote(storage, name, remote, ssh_auth: nil, forced: false, no_tags: false) - args = [gitlab_shell_projects_path, 'fetch-remote', storage, "#{name}.git", remote, "#{Gitlab.config.gitlab_shell.git_timeout}"] - args << '--force' if forced - args << '--no-tags' if no_tags - - vars = {} - - if ssh_auth&.ssh_import? - if ssh_auth.ssh_key_auth? && ssh_auth.ssh_private_key.present? - vars['GITLAB_SHELL_SSH_KEY'] = ssh_auth.ssh_private_key - end - - if ssh_auth.ssh_known_hosts.present? - vars['GITLAB_SHELL_KNOWN_HOSTS'] = ssh_auth.ssh_known_hosts + def fetch_remote(repository, remote, ssh_auth: nil, forced: false, no_tags: false) + gitaly_migrate(:fetch_remote) do |is_enabled| + if is_enabled + repository.gitaly_repository_client.fetch_remote(remote, ssh_auth: ssh_auth, forced: forced, no_tags: no_tags) + else + storage_path = Gitlab.config.repositories.storages[repository.storage]["path"] + local_fetch_remote(storage_path, repository.relative_path, remote, ssh_auth: ssh_auth, forced: forced, no_tags: no_tags) end end - - gitlab_shell_fast_execute_raise_error(args, vars) end # Move repository @@ -302,6 +293,26 @@ module Gitlab private + def local_fetch_remote(storage, name, remote, ssh_auth: nil, forced: false, no_tags: false) + args = [gitlab_shell_projects_path, 'fetch-remote', storage, name, remote, "#{Gitlab.config.gitlab_shell.git_timeout}"] + args << '--force' if forced + args << '--no-tags' if no_tags + + vars = {} + + if ssh_auth&.ssh_import? + if ssh_auth.ssh_key_auth? && ssh_auth.ssh_private_key.present? + vars['GITLAB_SHELL_SSH_KEY'] = ssh_auth.ssh_private_key + end + + if ssh_auth.ssh_known_hosts.present? + vars['GITLAB_SHELL_KNOWN_HOSTS'] = ssh_auth.ssh_known_hosts + end + end + + gitlab_shell_fast_execute_raise_error(args, vars) + end + def gitlab_shell_fast_execute(cmd) output, status = gitlab_shell_fast_execute_helper(cmd) @@ -325,5 +336,13 @@ module Gitlab # from wasting I/O by searching through GEM_PATH Bundler.with_original_env { Popen.popen(cmd, nil, vars) } end + + def gitaly_migrate(method, &block) + Gitlab::GitalyClient.migrate(method, &block) + rescue GRPC::NotFound, GRPC::BadStatus => e + # Old Popen code returns [Error, output] to the caller, so we + # need to do the same here... + raise Error, e + end end end diff --git a/spec/lib/gitlab/shell_spec.rb b/spec/lib/gitlab/shell_spec.rb index cfadee0bcf5..c7930378240 100644 --- a/spec/lib/gitlab/shell_spec.rb +++ b/spec/lib/gitlab/shell_spec.rb @@ -186,22 +186,48 @@ describe Gitlab::Shell do end end - describe '#fetch_remote' do + shared_examples 'fetch_remote' do |gitaly_on| + let(:project2) { create(:project, :repository) } + let(:repository) { project2.repository } + def fetch_remote(ssh_auth = nil) - gitlab_shell.fetch_remote('current/storage', 'project/path', 'new/storage', ssh_auth: ssh_auth) + gitlab_shell.fetch_remote(repository.raw_repository, 'new/storage', ssh_auth: ssh_auth) end - def expect_popen(vars = {}) + def expect_popen(fail = false, vars = {}) popen_args = [ projects_path, 'fetch-remote', - 'current/storage', - 'project/path.git', + TestEnv.repos_path, + repository.relative_path, 'new/storage', Gitlab.config.gitlab_shell.git_timeout.to_s ] - expect(Gitlab::Popen).to receive(:popen).with(popen_args, nil, popen_vars.merge(vars)) + return_value = fail ? ["error", 1] : [nil, 0] + + expect(Gitlab::Popen).to receive(:popen).with(popen_args, nil, popen_vars.merge(vars)).and_return(return_value) + end + + def expect_gitaly_call(fail, vars = {}) + receive_fetch_remote = + if fail + receive(:fetch_remote).and_raise(GRPC::NotFound) + else + receive(:fetch_remote).and_return(true) + end + + expect_any_instance_of(Gitlab::GitalyClient::RepositoryService).to receive_fetch_remote + end + + if gitaly_on + def expect_call(fail, vars = {}) + expect_gitaly_call(fail, vars) + end + else + def expect_call(fail, vars = {}) + expect_popen(fail, vars) + end end def build_ssh_auth(opts = {}) @@ -216,20 +242,20 @@ describe Gitlab::Shell do end it 'returns true when the command succeeds' do - expect_popen.and_return([nil, 0]) + expect_call(false) expect(fetch_remote).to be_truthy end it 'raises an exception when the command fails' do - expect_popen.and_return(["error", 1]) + expect_call(true) - expect { fetch_remote }.to raise_error(Gitlab::Shell::Error, "error") + expect { fetch_remote }.to raise_error(Gitlab::Shell::Error) end context 'SSH auth' do it 'passes the SSH key if specified' do - expect_popen('GITLAB_SHELL_SSH_KEY' => 'foo').and_return([nil, 0]) + expect_call(false, 'GITLAB_SHELL_SSH_KEY' => 'foo') ssh_auth = build_ssh_auth(ssh_key_auth?: true, ssh_private_key: 'foo') @@ -237,7 +263,7 @@ describe Gitlab::Shell do end it 'does not pass an empty SSH key' do - expect_popen.and_return([nil, 0]) + expect_call(false) ssh_auth = build_ssh_auth(ssh_key_auth: true, ssh_private_key: '') @@ -245,7 +271,7 @@ describe Gitlab::Shell do end it 'does not pass the key unless SSH key auth is to be used' do - expect_popen.and_return([nil, 0]) + expect_call(false) ssh_auth = build_ssh_auth(ssh_key_auth: false, ssh_private_key: 'foo') @@ -253,7 +279,7 @@ describe Gitlab::Shell do end it 'passes the known_hosts data if specified' do - expect_popen('GITLAB_SHELL_KNOWN_HOSTS' => 'foo').and_return([nil, 0]) + expect_call(false, 'GITLAB_SHELL_KNOWN_HOSTS' => 'foo') ssh_auth = build_ssh_auth(ssh_known_hosts: 'foo') @@ -261,7 +287,7 @@ describe Gitlab::Shell do end it 'does not pass empty known_hosts data' do - expect_popen.and_return([nil, 0]) + expect_call(false) ssh_auth = build_ssh_auth(ssh_known_hosts: '') @@ -269,7 +295,7 @@ describe Gitlab::Shell do end it 'does not pass known_hosts data unless SSH is to be used' do - expect_popen(popen_vars).and_return([nil, 0]) + expect_call(false, popen_vars) ssh_auth = build_ssh_auth(ssh_import?: false, ssh_known_hosts: 'foo') @@ -278,6 +304,14 @@ describe Gitlab::Shell do end end + describe '#fetch_remote local', skip_gitaly_mock: true do + it_should_behave_like 'fetch_remote', false + end + + describe '#fetch_remote gitaly' do + it_should_behave_like 'fetch_remote', true + end + describe '#import_repository' do it 'returns true when the command succeeds' do expect(Gitlab::Popen).to receive(:popen)