From 327a9898a226a098b18e80e4950702064ecd38f1 Mon Sep 17 00:00:00 2001 From: Nick Thomas Date: Thu, 30 Nov 2017 21:34:31 +0000 Subject: Fix the fork project functionality for projects with hashed storage Note the dependency on gitlab-shell v5.10.0 --- GITLAB_SHELL_VERSION | 2 +- app/models/project.rb | 3 +- app/workers/repository_fork_worker.rb | 12 ++-- .../40711-fix-forking-hashed-projects.yml | 5 ++ lib/gitlab/shell.rb | 23 ++++--- spec/lib/gitlab/shell_spec.rb | 8 +-- spec/models/project_spec.rb | 3 +- spec/workers/repository_fork_worker_spec.rb | 77 ++++++++-------------- 8 files changed, 61 insertions(+), 72 deletions(-) create mode 100644 changelogs/unreleased/40711-fix-forking-hashed-projects.yml diff --git a/GITLAB_SHELL_VERSION b/GITLAB_SHELL_VERSION index c5b7013b9c5..509b0b618ad 100644 --- a/GITLAB_SHELL_VERSION +++ b/GITLAB_SHELL_VERSION @@ -1 +1 @@ -5.9.4 +5.10.0 diff --git a/app/models/project.rb b/app/models/project.rb index eaf4f555d3b..f9c82029912 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -562,8 +562,7 @@ class Project < ActiveRecord::Base if forked? RepositoryForkWorker.perform_async(id, forked_from_project.repository_storage_path, - forked_from_project.full_path, - self.namespace.full_path) + forked_from_project.disk_path) else RepositoryImportWorker.perform_async(self.id) end diff --git a/app/workers/repository_fork_worker.rb b/app/workers/repository_fork_worker.rb index 264706e3e23..001c11b73e1 100644 --- a/app/workers/repository_fork_worker.rb +++ b/app/workers/repository_fork_worker.rb @@ -8,18 +8,18 @@ class RepositoryForkWorker sidekiq_options status_expiration: StuckImportJobsWorker::IMPORT_JOBS_EXPIRATION - def perform(project_id, forked_from_repository_storage_path, source_path, target_path) + def perform(project_id, forked_from_repository_storage_path, source_disk_path) project = Project.find(project_id) return unless start_fork(project) Gitlab::Metrics.add_event(:fork_repository, - source_path: source_path, - target_path: target_path) + source_path: source_disk_path, + target_path: project.disk_path) - result = gitlab_shell.fork_repository(forked_from_repository_storage_path, source_path, - project.repository_storage_path, target_path) - raise ForkError, "Unable to fork project #{project_id} for repository #{source_path} -> #{target_path}" unless result + result = gitlab_shell.fork_repository(forked_from_repository_storage_path, source_disk_path, + project.repository_storage_path, project.disk_path) + raise ForkError, "Unable to fork project #{project_id} for repository #{source_disk_path} -> #{project.disk_path}" unless result project.repository.after_import raise ForkError, "Project #{project_id} had an invalid repository after fork" unless project.valid_repo? diff --git a/changelogs/unreleased/40711-fix-forking-hashed-projects.yml b/changelogs/unreleased/40711-fix-forking-hashed-projects.yml new file mode 100644 index 00000000000..116d7d4e9cf --- /dev/null +++ b/changelogs/unreleased/40711-fix-forking-hashed-projects.yml @@ -0,0 +1,5 @@ +--- +title: Fix the fork project functionality for projects with hashed storage +merge_request: 15671 +author: +type: fixed diff --git a/lib/gitlab/shell.rb b/lib/gitlab/shell.rb index 996d213fdb4..a22a63665be 100644 --- a/lib/gitlab/shell.rb +++ b/lib/gitlab/shell.rb @@ -143,20 +143,27 @@ module Gitlab storage, "#{path}.git", "#{new_path}.git"]) end - # Fork repository to new namespace + # Fork repository to new path # forked_from_storage - forked-from project's storage path - # path - project path with namespace + # forked_from_disk_path - project disk path # forked_to_storage - forked-to project's storage path - # fork_namespace - namespace for forked project + # forked_to_disk_path - forked project disk path # # Ex. - # fork_repository("/path/to/forked_from/storage", "gitlab/gitlab-ci", "/path/to/forked_to/storage", "randx") + # fork_repository("/path/to/forked_from/storage", "gitlab/gitlab-ci", "/path/to/forked_to/storage", "new-namespace/gitlab-ci") # # Gitaly note: JV: not easy to migrate because this involves two Gitaly servers, not one. - def fork_repository(forked_from_storage, path, 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]) + def fork_repository(forked_from_storage, forked_from_disk_path, forked_to_storage, forked_to_disk_path) + gitlab_shell_fast_execute( + [ + gitlab_shell_projects_path, + 'fork-repository', + forked_from_storage, + "#{forked_from_disk_path}.git", + forked_to_storage, + "#{forked_to_disk_path}.git" + ] + ) end # Remove repository from file system diff --git a/spec/lib/gitlab/shell_spec.rb b/spec/lib/gitlab/shell_spec.rb index 2158b2837e2..eec6858a5de 100644 --- a/spec/lib/gitlab/shell_spec.rb +++ b/spec/lib/gitlab/shell_spec.rb @@ -200,18 +200,18 @@ describe Gitlab::Shell do 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'], + .with([projects_path, 'fork-repository', 'current/storage', 'project/path.git', 'new/storage', 'fork/path.git'], nil, popen_vars).and_return([nil, 0]) - expect(gitlab_shell.fork_repository('current/storage', 'project/path', 'new/storage', 'new-namespace')).to be true + expect(gitlab_shell.fork_repository('current/storage', 'project/path', 'new/storage', 'fork/path')).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'], + .with([projects_path, 'fork-repository', 'current/storage', 'project/path.git', 'new/storage', 'fork/path.git'], nil, popen_vars).and_return(["error", 1]) - expect(gitlab_shell.fork_repository('current/storage', 'project/path', 'new/storage', 'new-namespace')).to be false + expect(gitlab_shell.fork_repository('current/storage', 'project/path', 'new/storage', 'fork/path')).to be false end end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 521b7bd70ba..34e160aa599 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -1717,8 +1717,7 @@ describe Project do expect(RepositoryForkWorker).to receive(:perform_async).with( project.id, forked_from_project.repository_storage_path, - forked_from_project.disk_path, - project.namespace.full_path).and_return(import_jid) + forked_from_project.disk_path).and_return(import_jid) expect(project.add_import_job).to eq(import_jid) end diff --git a/spec/workers/repository_fork_worker_spec.rb b/spec/workers/repository_fork_worker_spec.rb index e881ec37ae5..74c85848b7e 100644 --- a/spec/workers/repository_fork_worker_spec.rb +++ b/spec/workers/repository_fork_worker_spec.rb @@ -1,8 +1,8 @@ require 'spec_helper' describe RepositoryForkWorker do - let(:project) { create(:project, :repository, :import_scheduled) } - let(:fork_project) { create(:project, :repository, forked_from_project: project) } + let(:project) { create(:project, :repository) } + let(:fork_project) { create(:project, :repository, :import_scheduled, forked_from_project: project) } let(:shell) { Gitlab::Shell.new } subject { described_class.new } @@ -12,50 +12,39 @@ describe RepositoryForkWorker do end describe "#perform" do + def perform! + subject.perform(fork_project.id, '/test/path', project.disk_path) + end + + def expect_fork_repository + expect(shell).to receive(:fork_repository).with( + '/test/path', + project.disk_path, + fork_project.repository_storage_path, + fork_project.disk_path + ) + end + describe 'when a worker was reset without cleanup' do let(:jid) { '12345678' } - let(:started_project) { create(:project, :repository, :import_started) } it 'creates a new repository from a fork' do allow(subject).to receive(:jid).and_return(jid) - expect(shell).to receive(:fork_repository).with( - '/test/path', - project.full_path, - project.repository_storage_path, - fork_project.namespace.full_path - ).and_return(true) - - subject.perform( - project.id, - '/test/path', - project.full_path, - fork_project.namespace.full_path) + expect_fork_repository.and_return(true) + + perform! end end it "creates a new repository from a fork" do - expect(shell).to receive(:fork_repository).with( - '/test/path', - project.full_path, - project.repository_storage_path, - fork_project.namespace.full_path - ).and_return(true) + expect_fork_repository.and_return(true) - subject.perform( - project.id, - '/test/path', - project.full_path, - fork_project.namespace.full_path) + perform! end it 'flushes various caches' do - expect(shell).to receive(:fork_repository).with( - '/test/path', - project.full_path, - project.repository_storage_path, - fork_project.namespace.full_path - ).and_return(true) + expect_fork_repository.and_return(true) expect_any_instance_of(Repository).to receive(:expire_emptiness_caches) .and_call_original @@ -63,32 +52,22 @@ describe RepositoryForkWorker do expect_any_instance_of(Repository).to receive(:expire_exists_cache) .and_call_original - subject.perform(project.id, '/test/path', project.full_path, - fork_project.namespace.full_path) + perform! end it "handles bad fork" do - source_path = project.full_path - target_path = fork_project.namespace.full_path - error_message = "Unable to fork project #{project.id} for repository #{source_path} -> #{target_path}" + error_message = "Unable to fork project #{fork_project.id} for repository #{project.full_path} -> #{fork_project.full_path}" - expect(shell).to receive(:fork_repository).and_return(false) + expect_fork_repository.and_return(false) - expect do - subject.perform(project.id, '/test/path', source_path, target_path) - end.to raise_error(RepositoryForkWorker::ForkError, error_message) + expect { perform! }.to raise_error(RepositoryForkWorker::ForkError, error_message) end it 'handles unexpected error' do - source_path = project.full_path - target_path = fork_project.namespace.full_path - - allow_any_instance_of(Gitlab::Shell).to receive(:fork_repository).and_raise(RuntimeError) + expect_fork_repository.and_raise(RuntimeError) - expect do - subject.perform(project.id, '/test/path', source_path, target_path) - end.to raise_error(RepositoryForkWorker::ForkError) - expect(project.reload.import_status).to eq('failed') + expect { perform! }.to raise_error(RepositoryForkWorker::ForkError) + expect(fork_project.reload.import_status).to eq('failed') end end end -- cgit v1.2.1