Merge branch '4099-remote-removal-port-to-background-job' into 'master'

Resolve "Disabling push mirror raises  error 500"

Closes #4099

See merge request gitlab-org/gitlab-ee!3971

app/models/repository.rb

@@ -1005,13 +1005,27 @@ class Repository
     add_remote(remote_name, url, mirror_refmap: refmap)
     fetch_remote(remote_name, forced: forced)
   ensure
-    remove_remote(remote_name) if tmp_remote_name
+    async_remove_remote(remote_name) if tmp_remote_name
   end
 
   def fetch_remote(remote, forced: false, ssh_auth: nil, no_tags: false)
     gitlab_shell.fetch_remote(raw_repository, remote, ssh_auth: ssh_auth, forced: forced, no_tags: no_tags)
   end
 
+  def async_remove_remote(remote_name)
+    return unless remote_name
+
+    job_id = RepositoryRemoveRemoteWorker.perform_async(project.id, remote_name)
+
+    if job_id
+      Rails.logger.info("Remove remote job scheduled for #{project.id} with remote name: #{remote_name} job ID #{job_id}.")
+    else
+      Rails.logger.info("Remove remote job failed to create for #{project.id} with remote name #{remote_name}.")
+    end
+
+    job_id
+  end
+
   def fetch_source_branch!(source_repository, source_branch, local_ref)
     raw_repository.fetch_source_branch!(source_repository.raw_repository, source_branch, local_ref)
   end

app/workers/all_queues.yml

@@ -93,6 +93,7 @@
 - rebase
 - repository_fork
 - repository_import
+- repository_remove_remote
 - storage_migrator
 - system_hook_push
 - update_merge_requests

app/workers/repository_remove_remote_worker.rb

+class RepositoryRemoveRemoteWorker
+  include ApplicationWorker
+  include ExclusiveLeaseGuard
+
+  LEASE_TIMEOUT = 1.hour
+
+  attr_reader :project, :remote_name
+
+  def perform(project_id, remote_name)
+    @remote_name = remote_name
+    @project = Project.find_by_id(project_id)
+
+    return unless @project
+
+    logger.info("Removing remote #{remote_name} from project #{project.id}")
+
+    try_obtain_lease do
+      remove_remote = @project.repository.remove_remote(remote_name)
+
+      if remove_remote
+        logger.info("Remote #{remote_name} was successfully removed from project #{project.id}")
+      else
+        logger.error("Could not remove remote #{remote_name} from project #{project.id}")
+      end
+    end
+  end
+
+  def lease_timeout
+    LEASE_TIMEOUT
+  end
+
+  def lease_key
+    "remove_remote_#{project.id}_#{remote_name}"
+  end
+end

changelogs/unreleased-ee/4099-remote-removal-port-to-background-job.yml

+---
+title: Ports remote removal to a background job
+merge_request:
+author:
+type: changed

config/sidekiq_queues.yml

@@ -40,6 +40,7 @@
   - [upload_checksum, 1]
   - [repository_fork, 1]
   - [repository_import, 1]
+  - [repository_remove_remote, 1]
   - [github_importer, 1]
   - [github_import_advance_stage, 1]
   - [project_service, 1]

db/migrate/20180109150457_add_remote_name_to_remote_mirrors.rb

+class AddRemoteNameToRemoteMirrors < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def change
+    add_column :remote_mirrors, :remote_name, :string
+  end
+end

db/schema.rb

@@ -2029,6 +2029,7 @@ ActiveRecord::Schema.define(version: 20180113220114) do
     t.datetime "updated_at", null: false
     t.datetime "last_update_started_at"
     t.boolean "only_protected_branches", default: false, null: false
+    t.string "remote_name"
   end
 
   add_index "remote_mirrors", ["last_successful_update_at"], name: "index_remote_mirrors_on_last_successful_update_at", using: :btree

ee/app/models/ee/project.rb

@@ -329,7 +329,7 @@ module EE
     end
 
     def remove_mirror_repository_reference
-      repository.remove_remote(::Repository::MIRROR_REMOTE)
+      repository.async_remove_remote(::Repository::MIRROR_REMOTE)
     end
 
     def import_url_availability

ee/app/models/remote_mirror.rb

@@ -21,6 +21,8 @@ class RemoteMirror < ActiveRecord::Base
   validate  :url_availability, if: -> (mirror) { mirror.url_changed? || mirror.enabled? }
   validates :url, addressable_url: true, if: :url_changed?
 
+  before_save :set_new_remote_name, if: :mirror_url_changed?
+
   after_save :set_override_remote_mirror_available, unless: -> { Gitlab::CurrentSettings.current_application_settings.mirror_available }
   after_save :refresh_remote, if: :mirror_url_changed?
   after_update :reset_fields, if: :mirror_url_changed?
@@ -69,8 +71,8 @@ class RemoteMirror < ActiveRecord::Base
     end
   end
 
-  def ref_name
-    "remote_mirror_#{id}"
+  def remote_name
+    super || fallback_remote_name
   end
 
   def update_failed?
@@ -149,7 +151,13 @@ class RemoteMirror < ActiveRecord::Base
   private
 
   def raw
-    @raw ||= Gitlab::Git::RemoteMirror.new(project.repository.raw, ref_name)
+    @raw ||= Gitlab::Git::RemoteMirror.new(project.repository.raw, remote_name)
+  end
+
+  def fallback_remote_name
+    return unless id
+
+    "remote_mirror_#{id}"
   end
 
   def recently_scheduled?
@@ -189,16 +197,27 @@ class RemoteMirror < ActiveRecord::Base
     project.update(remote_mirror_available_overridden: enabled)
   end
 
+  def set_new_remote_name
+    self.remote_name = "remote_mirror_#{SecureRandom.hex}"
+  end
+
   def refresh_remote
     return unless project
 
-    project.repository.add_remote(ref_name, url)
+    # Before adding a new remote we have to delete the data from
+    # the previous remote name
+    prev_remote_name = remote_name_was || fallback_remote_name
+    run_after_commit do
+      project.repository.async_remove_remote(prev_remote_name)
+    end
+
+    project.repository.add_remote(remote_name, url)
   end
 
   def remove_remote
-    if project # could be pending to delete so don't need to touch the git repository
-      project.repository.remove_remote(ref_name)
-    end
+    return unless project # could be pending to delete so don't need to touch the git repository
+
+    project.repository.async_remove_remote(remote_name)
   end
 
   def mirror_url_changed?

ee/app/services/projects/update_remote_mirror_service.rb

@@ -8,7 +8,7 @@ module Projects
       return success unless remote_mirror.enabled?
 
       begin
-        repository.fetch_remote(remote_mirror.ref_name, no_tags: true)
+        repository.fetch_remote(remote_mirror.remote_name, no_tags: true)
 
         opts = {}
         if remote_mirror.only_protected_branches?

spec/factories/projects.rb

@@ -177,6 +177,7 @@ FactoryBot.define do
 
     trait :remote_mirror do
       transient do
+        remote_name "remote_mirror_#{SecureRandom.hex}"
         url "http://foo.com"
         enabled true
       end

spec/models/remote_mirror_spec.rb

@@ -68,7 +68,36 @@ describe RemoteMirror do
         mirror.update_attribute(:url, 'http://foo:baz@test.com')
 
         config = repo.raw_repository.rugged.config
-        expect(config["remote.#{mirror.ref_name}.url"]).to eq('http://foo:baz@test.com')
+        expect(config["remote.#{mirror.remote_name}.url"]).to eq('http://foo:baz@test.com')
+      end
+    end
+  end
+
+  describe '#remote_name' do
+    context 'when remote name is persisted in the database' do
+      it 'returns remote name with random value' do
+        allow(SecureRandom).to receive(:hex).and_return('secret')
+
+        remote_mirror = create(:remote_mirror)
+
+        expect(remote_mirror.remote_name).to eq("remote_mirror_secret")
+      end
+    end
+
+    context 'when remote name is not persisted in the database' do
+      it 'returns remote name with remote mirror id' do
+        remote_mirror = create(:remote_mirror)
+        remote_mirror.remote_name = nil
+
+        expect(remote_mirror.remote_name).to eq("remote_mirror_#{remote_mirror.id}")
+      end
+    end
+
+    context 'when remote is not persisted in the database' do
+      it 'returns nil' do
+        remote_mirror = build(:remote_mirror, remote_name: nil)
+
+        expect(remote_mirror.remote_name).to be_nil
       end
     end
   end

spec/models/repository_spec.rb

@@ -701,6 +701,38 @@ describe Repository do
     end
   end
 
+  describe '#async_remove_remote' do
+    before do
+      masterrev = repository.find_branch('master').dereferenced_target
+      create_remote_branch('joe', 'remote_branch', masterrev)
+    end
+
+    context 'when worker is scheduled successfully' do
+      before do
+        masterrev = repository.find_branch('master').dereferenced_target
+        create_remote_branch('remote_name', 'remote_branch', masterrev)
+
+        allow(RepositoryRemoveRemoteWorker).to receive(:perform_async).and_return('1234')
+      end
+
+      it 'returns job_id' do
+        expect(repository.async_remove_remote('joe')).to eq('1234')
+      end
+    end
+
+    context 'when worker does not schedule successfully' do
+      before do
+        allow(RepositoryRemoveRemoteWorker).to receive(:perform_async).and_return(nil)
+      end
+
+      it 'returns nil' do
+        expect(Rails.logger).to receive(:info).with("Remove remote job failed to create for #{project.id} with remote name joe.")
+
+        expect(repository.async_remove_remote('joe')).to be_nil
+      end
+    end
+  end
+
   describe '#fetch_ref' do
     let(:broken_repository) { create(:project, :broken_storage).repository }
 

spec/workers/repository_remove_remote_worker_spec.rb

+require 'rails_helper'
+
+describe RepositoryRemoveRemoteWorker do
+  subject(:worker) { described_class.new }
+
+  describe '#perform' do
+    let(:remote_name) { 'joe'}
+    let!(:project) { create(:project, :repository) }
+
+    context 'when it cannot obtain lease' do
+      it 'logs error' do
+        allow_any_instance_of(Gitlab::ExclusiveLease).to receive(:try_obtain) { nil }
+
+        expect_any_instance_of(Repository).not_to receive(:remove_remote)
+        expect(worker).to receive(:log_error).with('Cannot obtain an exclusive lease. There must be another instance already in execution.')
+
+        worker.perform(project.id, remote_name)
+      end
+    end
+
+    context 'when it gets the lease' do
+      before do
+        allow_any_instance_of(Gitlab::ExclusiveLease).to receive(:try_obtain).and_return(true)
+      end
+
+      context 'when project does not exist' do
+        it 'returns nil' do
+          expect(worker.perform(-1, 'remote_name')).to be_nil
+        end
+      end
+
+      context 'when project exists' do
+        it 'removes remote from repository' do
+          masterrev = project.repository.find_branch('master').dereferenced_target
+
+          create_remote_branch(remote_name, 'remote_branch', masterrev)
+
+          expect_any_instance_of(Repository).to receive(:remove_remote).with(remote_name).and_call_original
+
+          worker.perform(project.id, remote_name)
+        end
+      end
+    end
+  end
+
+  def create_remote_branch(remote_name, branch_name, target)
+    rugged = project.repository.rugged
+    rugged.references.create("refs/remotes/#{remote_name}/#{branch_name}", target.id)
+  end
+end