Read about our upcoming Code of Conduct on this issue

Commit 9995f080 authored by Stan Hu's avatar Stan Hu
Browse files

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
parent a7eb1de3056a
...@@ -7,6 +7,7 @@ v 8.0.0 (unreleased) ...@@ -7,6 +7,7 @@ v 8.0.0 (unreleased)
- Fix emoji URLs in Markdown when relative_url_root is used (Stan Hu) - 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) - 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) - 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) - Prevent anchors from being hidden by header (Stan Hu)
- Fix bug where only the first 15 Bitbucket issues would be imported (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) - Sort issues by creation date in Bitbucket importer (Stan Hu)
......
...@@ -13,10 +13,14 @@ def create ...@@ -13,10 +13,14 @@ def create
@forked_project = ::Projects::ForkService.new(project, current_user, namespace: namespace).execute @forked_project = ::Projects::ForkService.new(project, current_user, namespace: namespace).execute
if @forked_project.saved? && @forked_project.forked? if @forked_project.saved? && @forked_project.forked?
redirect_to( if @forked_project.import_in_progress?
namespace_project_path(@forked_project.namespace, @forked_project), redirect_to namespace_project_import_path(@forked_project.namespace, @forked_project)
notice: 'Project was successfully forked.' else
) redirect_to(
namespace_project_path(@forked_project.namespace, @forked_project),
notice: 'Project was successfully forked.'
)
end
else else
render :error render :error
end end
......
...@@ -144,7 +144,7 @@ def set_last_activity_at ...@@ -144,7 +144,7 @@ def set_last_activity_at
validates_uniqueness_of :path, scope: :namespace_id validates_uniqueness_of :path, scope: :namespace_id
validates :import_url, validates :import_url,
format: { with: /\A#{URI.regexp(%w(ssh git http https))}\z/, message: 'should be a valid 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 } validates :star_count, numericality: { greater_than_or_equal_to: 0 }
validate :check_limit, on: :create validate :check_limit, on: :create
validate :avatar_type, validate :avatar_type,
...@@ -275,7 +275,13 @@ def saved? ...@@ -275,7 +275,13 @@ def saved?
end end
def add_import_job 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 end
def clear_import_data def clear_import_data
...@@ -283,6 +289,10 @@ def clear_import_data ...@@ -283,6 +289,10 @@ def clear_import_data
end end
def import? def import?
external_import? || forked?
end
def external_import?
import_url.present? import_url.present?
end end
...@@ -702,14 +712,8 @@ def origin_merge_requests ...@@ -702,14 +712,8 @@ def origin_merge_requests
end end
def create_repository def create_repository
if forked? # Forked import is handled asynchronously
if gitlab_shell.fork_repository(forked_from_project.path_with_namespace, self.namespace.path) unless forked?
true
else
errors.add(:base, 'Failed to fork repository via gitlab-shell')
false
end
else
if gitlab_shell.add_repository(path_with_namespace) if gitlab_shell.add_repository(path_with_namespace)
true true
else else
......
...@@ -55,9 +55,7 @@ def execute ...@@ -55,9 +55,7 @@ def execute
@project.save @project.save
if @project.persisted? && !@project.import? if @project.persisted? && !@project.import?
unless @project.create_repository raise 'Failed to create repository' unless @project.create_repository
raise 'Failed to create repository'
end
end end
end end
......
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
...@@ -15,7 +15,7 @@ class Spinach::Features::ProjectFork < Spinach::FeatureSteps ...@@ -15,7 +15,7 @@ class Spinach::Features::ProjectFork < Spinach::FeatureSteps
end end
step 'I should see the forked project page' do 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 end
step 'I already have a project named "Shop" in my namespace' do step 'I already have a project named "Shop" in my namespace' do
......
...@@ -96,6 +96,17 @@ ...@@ -96,6 +96,17 @@
expect(project.saved?).to be(true) expect(project.saved?).to be(true)
end end
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 end
def create_project(user, opts) def create_project(user, opts)
......
...@@ -28,8 +28,7 @@ ...@@ -28,8 +28,7 @@
context 'fork project failure' do context 'fork project failure' do
it "fails due to transaction failure" do it "fails due to transaction failure" do
@to_project = fork_project(@from_project, @to_user, false) @to_project = fork_project(@from_project, @to_user, false)
expect(@to_project.errors).not_to be_empty expect(@to_project.import_failed?)
expect(@to_project.errors[:base]).to include("Failed to fork repository via gitlab-shell")
end end
end end
...@@ -100,7 +99,7 @@ ...@@ -100,7 +99,7 @@
end end
def fork_project(from_project, user, fork_success = true, params = {}) 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 Projects::ForkService.new(from_project, user, params).execute
end end
end end
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
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment