diff --git a/GITLAB_SHELL_VERSION b/GITLAB_SHELL_VERSION index c5b7013b9c58a3e302b1645b3671211910c95622..509b0b618ad7fa28efedb6c2318a4c833f890f2b 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 eaf4f555d3bb50486673dc5d9379a26bc66244e4..f9c8202991241ca71730a0eac8063f21b31b9531 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 264706e3e23760a7303a8e00803778609fc857e0..001c11b73e1612b8a3e00e649eff2a25de6d086c 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 0000000000000000000000000000000000000000..116d7d4e9cf4d67496060ca2f51ba5022d013fc6 --- /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 996d213fdb410073fee0abb2fef9abea351e76f2..a22a63665be9ebc75913bb358a0b7f3766290072 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 2158b2837e2a8e4b231489afe5ed47d0714998c3..eec6858a5de4e755a25f0afccf6c8f25dfa6fd2e 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 521b7bd70bae94fed50d29e635bb43b31805fee3..34e160aa599242ec8df8c44ad28f90b6a65c5e2a 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 e881ec37ae587c7d8c51f84c1cab47efac7c8429..74c85848b7eaf2e34a0a11ef43fba6b319936482 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