Merge branch 'forked-repo-creation-async' into 'master'

Import forked repositories asynchronously to prevent large repositories from timing out

Preserve synchronous mode of adding a repository since some tasks (e.g. restore/check) require the former.

One side bug fix is that the `ProjectCacheWorker` is also run, updating the repository size and commit numbers properly. Previously all the values were set to 0 when a project were forked.

* Closes #2388
* Closes #2400

See merge request !1231

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)

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

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

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
 

app/workers/repository_fork_worker.rb

+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

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

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)

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

spec/workers/repository_fork_worker_spec.rb

+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