Moves remove_remote to a background job

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
+    schedule_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 schedule_remove_remote(remote_name)
+    return unless remote_name
+
+    job_id = RepositoryRemoveRemoteWorker.perform_async(project.id, remote_name)
+
+    if job_id
+      Rails.logger.info("RepositoryRemoveRemoteWorker job scheduled for #{project.id} with remote name: #{remote_name} job ID #{job_id}.")
+    else
+      Rails.logger.info("RepositoryRemoveRemoteWorker 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
+
+  LEASE_TIMEOUT = 1.hour
+
+  def perform(project_id, remote_name)
+    lease_uuid = try_obtain_lease(project_id)
+    return unless lease_uuid
+
+    project = Project.find(project_id)
+    project.repository.remove_remote(remote_name)
+
+    cancel_lease(project_id, lease_uuid)
+  end
+
+  private
+
+  def lease_key(project_id)
+    "remove_remote_#{project_id}"
+  end
+
+  def try_obtain_lease(id)
+    key = lease_key(id)
+
+    Gitlab::ExclusiveLease.new(key, timeout: LEASE_TIMEOUT).try_obtain
+  end
+
+  def cancel_lease(id, uuid)
+    key = lease_key(id)
+
+    Gitlab::ExclusiveLease.cancel(key, uuid)
+  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.schedule_remove_remote(::Repository::MIRROR_REMOTE)
     end
 
     def import_url_availability

ee/app/models/remote_mirror.rb

@@ -21,8 +21,9 @@ class RemoteMirror < ActiveRecord::Base
   validate  :url_availability, if: -> (mirror) { mirror.url_changed? || mirror.enabled? }
   validates :url, addressable_url: true, if: :url_changed?
 
+  before_save :refresh_remote, 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?
   after_destroy :remove_remote
 
@@ -69,7 +70,12 @@ class RemoteMirror < ActiveRecord::Base
     end
   end
 
-  def ref_name
+  def remote_name
+    name = read_attribute(:remote_name)
+
+    return name if name
+    return unless id
+
     "remote_mirror_#{id}"
   end
 
@@ -149,7 +155,7 @@ 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 recently_scheduled?
@@ -189,16 +195,28 @@ class RemoteMirror < ActiveRecord::Base
     project.update(remote_mirror_available_overridden: enabled)
   end
 
+  def write_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
+    run_after_commit do
+      project.repository.schedule_remove_remote(prev_remote_name)
+    end
+
+    write_new_remote_name
+    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.schedule_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 '#schedule_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.schedule_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("RepositoryRemoveRemoteWorker job failed to create for #{project.id} with remote name joe.")
+
+        expect(repository.schedule_remove_remote('joe')).to be_nil
+      end
+    end
+  end
+
   describe '#fetch_ref' do
     let(:broken_repository) { create(:project, :broken_storage).repository }
 

spec/services/projects/update_remote_mirror_service_spec.rb

@@ -27,15 +27,15 @@ describe Projects::UpdateRemoteMirrorService do
     end
 
     it "fetches the remote repository" do
-      expect(repository).to receive(:fetch_remote).with(remote_mirror.ref_name, no_tags: true) do
-        sync_remote(repository, remote_mirror.ref_name, local_branch_names)
+      expect(repository).to receive(:fetch_remote).with(remote_mirror.remote_name, no_tags: true) do
+        sync_remote(repository, remote_mirror.remote_name, local_branch_names)
       end
 
       subject.execute(remote_mirror)
     end
 
     it "succeeds" do
-      allow(repository).to receive(:fetch_remote) { sync_remote(repository, remote_mirror.ref_name, local_branch_names) }
+      allow(repository).to receive(:fetch_remote) { sync_remote(repository, remote_mirror.remote_name, local_branch_names) }
 
       result = subject.execute(remote_mirror)
 
@@ -46,13 +46,13 @@ describe Projects::UpdateRemoteMirrorService do
       it "push all the branches the first time" do
         allow(repository).to receive(:fetch_remote)
 
-        expect(raw_repository).to receive(:push_remote_branches).with(remote_mirror.ref_name, local_branch_names)
+        expect(raw_repository).to receive(:push_remote_branches).with(remote_mirror.remote_name, local_branch_names)
 
         subject.execute(remote_mirror)
       end
 
       it "does not push anything is remote is up to date" do
-        allow(repository).to receive(:fetch_remote) { sync_remote(repository, remote_mirror.ref_name, local_branch_names) }
+        allow(repository).to receive(:fetch_remote) { sync_remote(repository, remote_mirror.remote_name, local_branch_names) }
 
         expect(raw_repository).not_to receive(:push_remote_branches)
 
@@ -62,21 +62,21 @@ describe Projects::UpdateRemoteMirrorService do
       it "sync new branches" do
         # call local_branch_names early so it is not called after the new branch has been created
         current_branches = local_branch_names
-        allow(repository).to receive(:fetch_remote) { sync_remote(repository, remote_mirror.ref_name, current_branches) }
+        allow(repository).to receive(:fetch_remote) { sync_remote(repository, remote_mirror.remote_name, current_branches) }
         create_branch(repository, 'my-new-branch')
 
-        expect(raw_repository).to receive(:push_remote_branches).with(remote_mirror.ref_name, ['my-new-branch'])
+        expect(raw_repository).to receive(:push_remote_branches).with(remote_mirror.remote_name, ['my-new-branch'])
 
         subject.execute(remote_mirror)
       end
 
       it "sync updated branches" do
         allow(repository).to receive(:fetch_remote) do
-          sync_remote(repository, remote_mirror.ref_name, local_branch_names)
+          sync_remote(repository, remote_mirror.remote_name, local_branch_names)
           update_branch(repository, 'existing-branch')
         end
 
-        expect(raw_repository).to receive(:push_remote_branches).with(remote_mirror.ref_name, ['existing-branch'])
+        expect(raw_repository).to receive(:push_remote_branches).with(remote_mirror.remote_name, ['existing-branch'])
 
         subject.execute(remote_mirror)
       end
@@ -94,22 +94,22 @@ describe Projects::UpdateRemoteMirrorService do
 
         it "sync updated protected branches" do
           allow(repository).to receive(:fetch_remote) do
-            sync_remote(repository, remote_mirror.ref_name, local_branch_names)
+            sync_remote(repository, remote_mirror.remote_name, local_branch_names)
             update_branch(repository, protected_branch_name)
           end
 
-          expect(raw_repository).to receive(:push_remote_branches).with(remote_mirror.ref_name, [protected_branch_name])
+          expect(raw_repository).to receive(:push_remote_branches).with(remote_mirror.remote_name, [protected_branch_name])
 
           subject.execute(remote_mirror)
         end
 
         it 'does not sync unprotected branches' do
           allow(repository).to receive(:fetch_remote) do
-            sync_remote(repository, remote_mirror.ref_name, local_branch_names)
+            sync_remote(repository, remote_mirror.remote_name, local_branch_names)
             update_branch(repository, 'existing-branch')
           end
 
-          expect(raw_repository).not_to receive(:push_remote_branches).with(remote_mirror.ref_name, ['existing-branch'])
+          expect(raw_repository).not_to receive(:push_remote_branches).with(remote_mirror.remote_name, ['existing-branch'])
 
           subject.execute(remote_mirror)
         end
@@ -119,11 +119,11 @@ describe Projects::UpdateRemoteMirrorService do
         context 'when it has diverged' do
           it 'syncs branches' do
             allow(repository).to receive(:fetch_remote) do
-              sync_remote(repository, remote_mirror.ref_name, local_branch_names)
-              update_remote_branch(repository, remote_mirror.ref_name, 'markdown')
+              sync_remote(repository, remote_mirror.remote_name, local_branch_names)
+              update_remote_branch(repository, remote_mirror.remote_name, 'markdown')
             end
 
-            expect(raw_repository).to receive(:push_remote_branches).with(remote_mirror.ref_name, ['markdown'])
+            expect(raw_repository).to receive(:push_remote_branches).with(remote_mirror.remote_name, ['markdown'])
 
             subject.execute(remote_mirror)
           end
@@ -134,11 +134,11 @@ describe Projects::UpdateRemoteMirrorService do
         context 'when branch exists in local and remote repo' do
           it 'deletes the branch from remote repo' do
             allow(repository).to receive(:fetch_remote) do
-              sync_remote(repository, remote_mirror.ref_name, local_branch_names)
+              sync_remote(repository, remote_mirror.remote_name, local_branch_names)
               delete_branch(repository, 'existing-branch')
             end
 
-            expect(raw_repository).to receive(:delete_remote_branches).with(remote_mirror.ref_name, ['existing-branch'])
+            expect(raw_repository).to receive(:delete_remote_branches).with(remote_mirror.remote_name, ['existing-branch'])
 
             subject.execute(remote_mirror)
           end
@@ -159,22 +159,22 @@ describe Projects::UpdateRemoteMirrorService do
 
             it 'deletes the protected branch from remote repo' do
               allow(repository).to receive(:fetch_remote) do
-                sync_remote(repository, remote_mirror.ref_name, local_branch_names)
+                sync_remote(repository, remote_mirror.remote_name, local_branch_names)
                 delete_branch(repository, protected_branch_name)
               end
 
-              expect(raw_repository).not_to receive(:delete_remote_branches).with(remote_mirror.ref_name, [protected_branch_name])
+              expect(raw_repository).not_to receive(:delete_remote_branches).with(remote_mirror.remote_name, [protected_branch_name])
 
               subject.execute(remote_mirror)
             end
 
             it 'does not delete the unprotected branch from remote repo' do
               allow(repository).to receive(:fetch_remote) do
-                sync_remote(repository, remote_mirror.ref_name, local_branch_names)
+                sync_remote(repository, remote_mirror.remote_name, local_branch_names)
                 delete_branch(repository, 'existing-branch')
               end
 
-              expect(raw_repository).not_to receive(:delete_remote_branches).with(remote_mirror.ref_name, ['existing-branch'])
+              expect(raw_repository).not_to receive(:delete_remote_branches).with(remote_mirror.remote_name, ['existing-branch'])
 
               subject.execute(remote_mirror)
             end
@@ -190,10 +190,10 @@ describe Projects::UpdateRemoteMirrorService do
             context 'when it has diverged' do
               it 'does not delete the remote branch' do
                 allow(repository).to receive(:fetch_remote) do
-                  sync_remote(repository, remote_mirror.ref_name, local_branch_names)
+                  sync_remote(repository, remote_mirror.remote_name, local_branch_names)
 
                   rev = repository.find_branch('markdown').dereferenced_target
-                  create_remote_branch(repository, remote_mirror.ref_name, 'remote-branch', rev.id)
+                  create_remote_branch(repository, remote_mirror.remote_name, 'remote-branch', rev.id)
                 end
 
                 expect(raw_repository).not_to receive(:delete_remote_branches)
@@ -205,13 +205,13 @@ describe Projects::UpdateRemoteMirrorService do
             context 'when it has not diverged' do
               it 'deletes the remote branch' do
                 allow(repository).to receive(:fetch_remote) do
-                  sync_remote(repository, remote_mirror.ref_name, local_branch_names)
+                  sync_remote(repository, remote_mirror.remote_name, local_branch_names)
 
                   masterrev = repository.find_branch('master').dereferenced_target
-                  create_remote_branch(repository, remote_mirror.ref_name, protected_branch_name, masterrev.id)
+                  create_remote_branch(repository, remote_mirror.remote_name, protected_branch_name, masterrev.id)
                 end
 
-                expect(raw_repository).to receive(:delete_remote_branches).with(remote_mirror.ref_name, [protected_branch_name])
+                expect(raw_repository).to receive(:delete_remote_branches).with(remote_mirror.remote_name, [protected_branch_name])
 
                 subject.execute(remote_mirror)
               end
@@ -223,10 +223,10 @@ describe Projects::UpdateRemoteMirrorService do
           context 'when it has diverged' do
             it 'does not delete the remote branch' do
               allow(repository).to receive(:fetch_remote) do
-                sync_remote(repository, remote_mirror.ref_name, local_branch_names)
+                sync_remote(repository, remote_mirror.remote_name, local_branch_names)
 
                 rev = repository.find_branch('markdown').dereferenced_target
-                create_remote_branch(repository, remote_mirror.ref_name, 'remote-branch', rev.id)
+                create_remote_branch(repository, remote_mirror.remote_name, 'remote-branch', rev.id)
               end
 
               expect(raw_repository).not_to receive(:delete_remote_branches)
@@ -238,13 +238,13 @@ describe Projects::UpdateRemoteMirrorService do
           context 'when it has not diverged' do
             it 'deletes the remote branch' do
               allow(repository).to receive(:fetch_remote) do
-                sync_remote(repository, remote_mirror.ref_name, local_branch_names)
+                sync_remote(repository, remote_mirror.remote_name, local_branch_names)
 
                 masterrev = repository.find_branch('master').dereferenced_target
-                create_remote_branch(repository, remote_mirror.ref_name, 'remote-branch', masterrev.id)
+                create_remote_branch(repository, remote_mirror.remote_name, 'remote-branch', masterrev.id)
               end
 
-              expect(raw_repository).to receive(:delete_remote_branches).with(remote_mirror.ref_name, ['remote-branch'])
+              expect(raw_repository).to receive(:delete_remote_branches).with(remote_mirror.remote_name, ['remote-branch'])
 
               subject.execute(remote_mirror)
             end
@@ -255,7 +255,7 @@ describe Projects::UpdateRemoteMirrorService do
 
     describe 'Syncing tags' do
       before do
-        allow(repository).to receive(:fetch_remote) { sync_remote(repository, remote_mirror.ref_name, local_branch_names) }
+        allow(repository).to receive(:fetch_remote) { sync_remote(repository, remote_mirror.remote_name, local_branch_names) }
       end
 
       context 'when there are not tags to push' do
@@ -273,7 +273,7 @@ describe Projects::UpdateRemoteMirrorService do
         it 'pushes tags to remote' do
           allow(raw_repository).to receive(:remote_tags) { {} }
 
-          expect(raw_repository).to receive(:push_remote_branches).with(remote_mirror.ref_name, ['v1.0.0', 'v1.1.0'])
+          expect(raw_repository).to receive(:push_remote_branches).with(remote_mirror.remote_name, ['v1.0.0', 'v1.1.0'])
 
           subject.execute(remote_mirror)
         end
@@ -286,7 +286,7 @@ describe Projects::UpdateRemoteMirrorService do
 
           repository.rm_tag(create(:user), 'v1.0.0')
 
-          expect(raw_repository).to receive(:delete_remote_branches).with(remote_mirror.ref_name, ['v1.0.0'])
+          expect(raw_repository).to receive(:delete_remote_branches).with(remote_mirror.remote_name, ['v1.0.0'])
 
           subject.execute(remote_mirror)
         end

spec/workers/repository_remove_remote_worker_spec.rb

+require 'rails_helper'
+
+describe RepositoryRemoveRemoteWorker do
+  describe '#perform' do
+    let(:remote_name) { 'joe'}
+    let!(:project) { create(:project, :repository) }
+
+    context 'when it does not get the lease' do
+      it 'does not execute' do
+        allow_any_instance_of(Gitlab::ExclusiveLease).to receive(:try_obtain).and_return(false)
+
+        expect_any_instance_of(Repository).not_to receive(:remove_remote)
+
+        subject.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 { subject.perform(-1, 'remote_name') }.to raise_error(ActiveRecord::RecordNotFound)
+        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
+
+          subject.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