From 35b9274f12f29524855237bfdcd864497d62de95 Mon Sep 17 00:00:00 2001
From: Jacob Vosmaer <jacob@gitlab.com>
Date: Tue, 2 Apr 2019 13:20:26 +0000
Subject: [PATCH] Stop calling UnlinkRepositoryFromObjectPool RPC

Fixes https://gitlab.com/gitlab-org/gitlab-ce/issues/59777.

In earlier iterations of our implementation of Git object deduplication
we thought we would be making extensive use of Git remotes in pool
repositories in the future, and that we should manage these remotes
carefully from the start. We now expect we only care about one remote,
namely the source project. The other remotes are there only for forensic
purposes.

Before this MR we tried to also remove pool remotes when member projects
got deleted, with the UnlinkRepositoryFromObjectPool RPC. This is
fragile when there are race conditions (see
https://gitlab.com/gitlab-org/gitaly/issues/1568#note_153955926). We
have spent some time making this RPC less fragile in
https://gitlab.com/gitlab-org/gitaly/merge_requests/1151 but looking at
this problem again, I think we should just stop calling it.
---
 app/models/pool_repository.rb                   |  5 +----
 app/models/project.rb                           |  2 +-
 lib/gitlab/git/object_pool.rb                   |  2 +-
 lib/gitlab/gitaly_client/object_pool_service.rb | 10 ----------
 spec/models/pool_repository_spec.rb             |  6 +++---
 5 files changed, 6 insertions(+), 19 deletions(-)

diff --git a/app/models/pool_repository.rb b/app/models/pool_repository.rb
index 35c718365b4..7934118761e 100644
--- a/app/models/pool_repository.rb
+++ b/app/models/pool_repository.rb
@@ -81,10 +81,7 @@ class PoolRepository < ApplicationRecord
     object_pool.link(repository.raw)
   end
 
-  # This RPC can cause data loss, as not all objects are present the local repository
-  def unlink_repository(repository)
-    object_pool.unlink_repository(repository.raw)
-
+  def mark_obsolete_if_last(repository)
     if member_projects.where.not(id: repository.project.id).exists?
       true
     else
diff --git a/app/models/project.rb b/app/models/project.rb
index 82c2f9090c8..e2869fc2ad5 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -2128,7 +2128,7 @@ class Project < ApplicationRecord
   end
 
   def leave_pool_repository
-    pool_repository&.unlink_repository(repository) && update_column(:pool_repository_id, nil)
+    pool_repository&.mark_obsolete_if_last(repository) && update_column(:pool_repository_id, nil)
   end
 
   def link_pool_repository
diff --git a/lib/gitlab/git/object_pool.rb b/lib/gitlab/git/object_pool.rb
index e93ca3e11f8..8eb3c28ab70 100644
--- a/lib/gitlab/git/object_pool.rb
+++ b/lib/gitlab/git/object_pool.rb
@@ -8,7 +8,7 @@ module Gitlab
       GL_REPOSITORY = ""
 
       delegate :exists?, :size, to: :repository
-      delegate :unlink_repository, :delete, to: :object_pool_service
+      delegate :delete, to: :object_pool_service
 
       attr_reader :storage, :relative_path, :source_repository, :gl_project_path
 
diff --git a/lib/gitlab/gitaly_client/object_pool_service.rb b/lib/gitlab/gitaly_client/object_pool_service.rb
index 6e7ede5fd18..ce1fb4d68ae 100644
--- a/lib/gitlab/gitaly_client/object_pool_service.rb
+++ b/lib/gitlab/gitaly_client/object_pool_service.rb
@@ -33,16 +33,6 @@ module Gitlab
         GitalyClient.call(storage, :object_pool_service, :link_repository_to_object_pool,
                           request, timeout: GitalyClient.fast_timeout)
       end
-
-      def unlink_repository(repository)
-        request = Gitaly::UnlinkRepositoryFromObjectPoolRequest.new(
-          object_pool: object_pool,
-          repository: repository.gitaly_repository
-        )
-
-        GitalyClient.call(storage, :object_pool_service, :unlink_repository_from_object_pool,
-                          request, timeout: GitalyClient.fast_timeout)
-      end
     end
   end
 end
diff --git a/spec/models/pool_repository_spec.rb b/spec/models/pool_repository_spec.rb
index 112d4ab56fc..e5a3a3ad66e 100644
--- a/spec/models/pool_repository_spec.rb
+++ b/spec/models/pool_repository_spec.rb
@@ -24,14 +24,14 @@ describe PoolRepository do
     end
   end
 
-  describe '#unlink_repository' do
+  describe '#mark_obsolete_if_last' do
     let(:pool) { create(:pool_repository, :ready) }
 
     context 'when the last member leaves' do
       it 'schedules pool removal' do
         expect(::ObjectPool::DestroyWorker).to receive(:perform_async).with(pool.id).and_call_original
 
-        pool.unlink_repository(pool.source_project.repository)
+        pool.mark_obsolete_if_last(pool.source_project.repository)
       end
     end
 
@@ -40,7 +40,7 @@ describe PoolRepository do
         create(:project, :repository, pool_repository: pool)
         expect(::ObjectPool::DestroyWorker).not_to receive(:perform_async).with(pool.id)
 
-        pool.unlink_repository(pool.source_project.repository)
+        pool.mark_obsolete_if_last(pool.source_project.repository)
       end
     end
   end