From 9995f0806b29934cf498607f59d2c5ec358a0d5a Mon Sep 17 00:00:00 2001
From: Stan Hu <stanhu@gmail.com>
Date: Tue, 1 Sep 2015 00:56:40 -0700
Subject: [PATCH] Import forked repositories asynchronously to prevent large
repositories from timing out
Use import_status to track async import status and give feedback to the user
Closes #2388
Closes #2400
---
CHANGELOG | 1 +
app/controllers/projects/forks_controller.rb | 12 ++++---
app/models/project.rb | 24 +++++++------
app/services/projects/create_service.rb | 4 +--
app/workers/repository_fork_worker.rb | 34 +++++++++++++++++++
features/steps/project/fork.rb | 2 +-
spec/services/projects/create_service_spec.rb | 11 ++++++
spec/services/projects/fork_service_spec.rb | 5 ++-
spec/workers/repository_fork_worker_spec.rb | 29 ++++++++++++++++
9 files changed, 101 insertions(+), 21 deletions(-)
create mode 100644 app/workers/repository_fork_worker.rb
create mode 100644 spec/workers/repository_fork_worker_spec.rb
diff --git a/CHANGELOG b/CHANGELOG
index 2ae83d5b3ce..ddfd384f8c8 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -7,6 +7,7 @@ v 8.0.0 (unreleased)
- Fix emoji URLs in Markdown when relative_url_root is used (Stan Hu)
- Omit filename in Content-Disposition header in raw file download to avoid RFC 6266 encoding issues (Stan HU)
- Fix broken Wiki Page History (Stan Hu)
+ - Import forked repositories asynchronously to prevent large repositories from timing out (Stan Hu)
- Prevent anchors from being hidden by header (Stan Hu)
- Fix bug where only the first 15 Bitbucket issues would be imported (Stan Hu)
- Sort issues by creation date in Bitbucket importer (Stan Hu)
diff --git a/app/controllers/projects/forks_controller.rb b/app/controllers/projects/forks_controller.rb
index 9e72597ea87..8a785076bb7 100644
--- a/app/controllers/projects/forks_controller.rb
+++ b/app/controllers/projects/forks_controller.rb
@@ -13,10 +13,14 @@ class Projects::ForksController < Projects::ApplicationController
@forked_project = ::Projects::ForkService.new(project, current_user, namespace: namespace).execute
if @forked_project.saved? && @forked_project.forked?
- redirect_to(
- namespace_project_path(@forked_project.namespace, @forked_project),
- notice: 'Project was successfully forked.'
- )
+ if @forked_project.import_in_progress?
+ redirect_to namespace_project_import_path(@forked_project.namespace, @forked_project)
+ else
+ redirect_to(
+ namespace_project_path(@forked_project.namespace, @forked_project),
+ notice: 'Project was successfully forked.'
+ )
+ end
else
render :error
end
diff --git a/app/models/project.rb b/app/models/project.rb
index 072d7d73f41..49525eb9227 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -144,7 +144,7 @@ class Project < ActiveRecord::Base
validates_uniqueness_of :path, scope: :namespace_id
validates :import_url,
format: { with: /\A#{URI.regexp(%w(ssh git http https))}\z/, message: 'should be a valid url' },
- if: :import?
+ if: :external_import?
validates :star_count, numericality: { greater_than_or_equal_to: 0 }
validate :check_limit, on: :create
validate :avatar_type,
@@ -275,7 +275,13 @@ class Project < ActiveRecord::Base
end
def add_import_job
- RepositoryImportWorker.perform_in(2.seconds, id)
+ if forked?
+ unless RepositoryForkWorker.perform_async(id, forked_from_project.path_with_namespace, self.namespace.path)
+ import_fail
+ end
+ else
+ RepositoryImportWorker.perform_in(2.seconds, id)
+ end
end
def clear_import_data
@@ -283,6 +289,10 @@ class Project < ActiveRecord::Base
end
def import?
+ external_import? || forked?
+ end
+
+ def external_import?
import_url.present?
end
@@ -702,14 +712,8 @@ class Project < ActiveRecord::Base
end
def create_repository
- if forked?
- if gitlab_shell.fork_repository(forked_from_project.path_with_namespace, self.namespace.path)
- true
- else
- errors.add(:base, 'Failed to fork repository via gitlab-shell')
- false
- end
- else
+ # Forked import is handled asynchronously
+ unless forked?
if gitlab_shell.add_repository(path_with_namespace)
true
else
diff --git a/app/services/projects/create_service.rb b/app/services/projects/create_service.rb
index 1bb2462565a..e54a13ed6c5 100644
--- a/app/services/projects/create_service.rb
+++ b/app/services/projects/create_service.rb
@@ -55,9 +55,7 @@ module Projects
@project.save
if @project.persisted? && !@project.import?
- unless @project.create_repository
- raise 'Failed to create repository'
- end
+ raise 'Failed to create repository' unless @project.create_repository
end
end
diff --git a/app/workers/repository_fork_worker.rb b/app/workers/repository_fork_worker.rb
new file mode 100644
index 00000000000..acd1c43f06b
--- /dev/null
+++ b/app/workers/repository_fork_worker.rb
@@ -0,0 +1,34 @@
+class RepositoryForkWorker
+ include Sidekiq::Worker
+ include Gitlab::ShellAdapter
+
+ sidekiq_options queue: :gitlab_shell
+
+ def perform(project_id, source_path, target_path)
+ project = Project.find_by_id(project_id)
+
+ unless project.present?
+ logger.error("Project #{project_id} no longer exists!")
+ return
+ end
+
+ result = gitlab_shell.fork_repository(source_path, target_path)
+
+ unless result
+ logger.error("Unable to fork project #{project_id} for repository #{source_path} -> #{target_path}")
+ project.import_fail
+ project.save
+ return
+ end
+
+ if project.valid_repo?
+ ProjectCacheWorker.perform_async(project.id)
+ project.import_finish
+ else
+ project.import_fail
+ logger.error("Project #{id} had an invalid repository after fork")
+ end
+
+ project.save
+ end
+end
diff --git a/features/steps/project/fork.rb b/features/steps/project/fork.rb
index 0e433781d7a..370960845cc 100644
--- a/features/steps/project/fork.rb
+++ b/features/steps/project/fork.rb
@@ -15,7 +15,7 @@ class Spinach::Features::ProjectFork < Spinach::FeatureSteps
end
step 'I should see the forked project page' do
- expect(page).to have_content "Project was successfully forked."
+ expect(page).to have_content "Forked from"
end
step 'I already have a project named "Shop" in my namespace' do
diff --git a/spec/services/projects/create_service_spec.rb b/spec/services/projects/create_service_spec.rb
index ff4ed2dd484..25277f07482 100644
--- a/spec/services/projects/create_service_spec.rb
+++ b/spec/services/projects/create_service_spec.rb
@@ -96,6 +96,17 @@ describe Projects::CreateService do
expect(project.saved?).to be(true)
end
end
+
+ context 'repository creation' do
+ it 'should synchronously create the repository' do
+ expect_any_instance_of(Project).to receive(:create_repository)
+
+ project = create_project(@user, @opts)
+ expect(project).to be_valid
+ expect(project.owner).to eq(@user)
+ expect(project.namespace).to eq(@user.namespace)
+ end
+ end
end
def create_project(user, opts)
diff --git a/spec/services/projects/fork_service_spec.rb b/spec/services/projects/fork_service_spec.rb
index c04e842c67e..7c4bb74b77f 100644
--- a/spec/services/projects/fork_service_spec.rb
+++ b/spec/services/projects/fork_service_spec.rb
@@ -28,8 +28,7 @@ describe Projects::ForkService do
context 'fork project failure' do
it "fails due to transaction failure" do
@to_project = fork_project(@from_project, @to_user, false)
- expect(@to_project.errors).not_to be_empty
- expect(@to_project.errors[:base]).to include("Failed to fork repository via gitlab-shell")
+ expect(@to_project.import_failed?)
end
end
@@ -100,7 +99,7 @@ describe Projects::ForkService do
end
def fork_project(from_project, user, fork_success = true, params = {})
- allow_any_instance_of(Gitlab::Shell).to receive(:fork_repository).and_return(fork_success)
+ allow(RepositoryForkWorker).to receive(:perform_async).and_return(fork_success)
Projects::ForkService.new(from_project, user, params).execute
end
end
diff --git a/spec/workers/repository_fork_worker_spec.rb b/spec/workers/repository_fork_worker_spec.rb
new file mode 100644
index 00000000000..aa031106968
--- /dev/null
+++ b/spec/workers/repository_fork_worker_spec.rb
@@ -0,0 +1,29 @@
+require 'spec_helper'
+
+describe RepositoryForkWorker do
+ let(:project) { create(:project) }
+ let(:fork_project) { create(:project, forked_from_project: project) }
+
+ subject { RepositoryForkWorker.new }
+
+ describe "#perform" do
+ it "creates a new repository from a fork" do
+ expect_any_instance_of(Gitlab::Shell).to receive(:fork_repository).with(
+ project.path_with_namespace,
+ fork_project.namespace.path).
+ and_return(true)
+ expect(ProjectCacheWorker).to receive(:perform_async)
+
+ subject.perform(project.id,
+ project.path_with_namespace,
+ fork_project.namespace.path)
+ end
+
+ it "handles bad fork" do
+ expect_any_instance_of(Gitlab::Shell).to receive(:fork_repository).and_return(false)
+ subject.perform(project.id,
+ project.path_with_namespace,
+ fork_project.namespace.path)
+ end
+ end
+end
--
GitLab