Merge branch 'zj-pool-destruction' into 'master'

Leave object pools when destroying projects

Closes gitaly#1415

See merge request gitlab-org/gitlab-ce!23869

Gemfile

@@ -419,7 +419,7 @@ group :ed25519 do
 end
 
 # Gitaly GRPC client
-gem 'gitaly-proto', '~> 1.3.0', require: 'gitaly'
+gem 'gitaly-proto', '~> 1.5.0', require: 'gitaly'
 gem 'grpc', '~> 1.15.0'
 
 gem 'google-protobuf', '~> 3.6'

Gemfile.lock

@@ -274,7 +274,7 @@ GEM
       gettext_i18n_rails (>= 0.7.1)
       po_to_json (>= 1.0.0)
       rails (>= 3.2.0)
-    gitaly-proto (1.3.0)
+    gitaly-proto (1.5.0)
       grpc (~> 1.0)
     github-markup (1.7.0)
     gitlab-default_value_for (3.1.1)
@@ -1008,7 +1008,7 @@ DEPENDENCIES
   gettext (~> 3.2.2)
   gettext_i18n_rails (~> 1.8.0)
   gettext_i18n_rails_js (~> 1.3)
-  gitaly-proto (~> 1.3.0)
+  gitaly-proto (~> 1.5.0)
   github-markup (~> 1.7.0)
   gitlab-default_value_for (~> 3.1.1)
   gitlab-markup (~> 1.6.5)

app/models/pool_repository.rb

@@ -18,6 +18,7 @@ class PoolRepository < ActiveRecord::Base
     state :scheduled
     state :ready
     state :failed
+    state :obsolete
 
     event :schedule do
       transition none: :scheduled
@@ -31,6 +32,10 @@ class PoolRepository < ActiveRecord::Base
       transition all => :failed
     end
 
+    event :mark_obsolete do
+      transition all => :obsolete
+    end
+
     state all - [:ready] do
       def joinable?
         false
@@ -54,6 +59,12 @@ class PoolRepository < ActiveRecord::Base
         ::ObjectPool::ScheduleJoinWorker.perform_async(pool.id)
       end
     end
+
+    after_transition any => :obsolete do |pool, _|
+      pool.run_after_commit do
+        ::ObjectPool::DestroyWorker.perform_async(pool.id)
+      end
+    end
   end
 
   def create_object_pool
@@ -71,10 +82,10 @@ class PoolRepository < ActiveRecord::Base
   end
 
   # This RPC can cause data loss, as not all objects are present the local repository
-  # No execution path yet, will be added through:
-  # https://gitlab.com/gitlab-org/gitaly/issues/1415
-  def delete_repository_alternate(repository)
+  def unlink_repository(repository)
     object_pool.unlink_repository(repository.raw)
+
+    mark_obsolete unless member_projects.where.not(id: repository.project.id).exists?
   end
 
   def object_pool

app/models/project.rb

@@ -2004,6 +2004,10 @@ class Project < ActiveRecord::Base
       Feature.enabled?(:object_pools, self)
   end
 
+  def leave_pool_repository
+    pool_repository&.unlink_repository(repository)
+  end
+
   private
 
   def create_new_pool_repository

app/services/projects/destroy_service.rb

@@ -137,6 +137,8 @@ module Projects
         raise_error('Failed to remove some tags in project container registry. Please try again or contact administrator.')
       end
 
+      project.leave_pool_repository
+
       Project.transaction do
         log_destroy_event
         trash_repositories!

app/workers/all_queues.yml

@@ -88,6 +88,7 @@
 - object_pool:object_pool_create
 - object_pool:object_pool_schedule_join
 - object_pool:object_pool_join
+- object_pool:object_pool_destroy
 
 - default
 - mailers # ActionMailer::DeliveryJob.queue_name

app/workers/object_pool/destroy_worker.rb

+# frozen_string_literal: true
+
+module ObjectPool
+  class DestroyWorker
+    include ApplicationWorker
+    include ObjectPoolQueue
+
+    def perform(pool_repository_id)
+      pool = PoolRepository.find_by_id(pool_repository_id)
+      return unless pool&.obsolete?
+
+      pool.delete_object_pool
+      pool.destroy
+    end
+  end
+end

lib/gitlab/git/object_pool.rb

@@ -8,7 +8,7 @@ module Gitlab
       GL_REPOSITORY = ""
 
       delegate :exists?, :size, to: :repository
-      delegate :delete, to: :object_pool_service
+      delegate :unlink_repository, :delete, to: :object_pool_service
 
       attr_reader :storage, :relative_path, :source_repository
 

lib/gitlab/gitaly_client/object_pool_service.rb

@@ -35,7 +35,10 @@ module Gitlab
       end
 
       def unlink_repository(repository)
-        request = Gitaly::UnlinkRepositoryFromObjectPoolRequest.new(repository: repository.gitaly_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)

spec/factories/pool_repositories.rb

@@ -15,6 +15,10 @@ FactoryBot.define do
       state :failed
     end
 
+    trait :obsolete do
+      state :obsolete
+    end
+
     trait :ready do
       state :ready
 

spec/models/pool_repository_spec.rb

@@ -23,4 +23,25 @@ describe PoolRepository do
       expect(pool.disk_path).to match(%r{\A@pools/\h{2}/\h{2}/\h{64}})
     end
   end
+
+  describe '#unlink_repository' 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)
+      end
+    end
+
+    context 'when the second member leaves' do
+      it 'does not schedule pool removal' 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)
+      end
+    end
+  end
 end

spec/workers/object_pool/destroy_worker_spec.rb

+# frozen_string_literal: true
+
+describe ObjectPool::DestroyWorker do
+  describe '#perform' do
+    context 'when no pool is in the database' do
+      it "doesn't raise an error" do
+        expect do
+          described_class.new.perform(987654321)
+        end.not_to raise_error
+      end
+    end
+
+    context 'when a pool is present' do
+      let(:pool) { create(:pool_repository, :obsolete) }
+
+      subject { described_class.new }
+
+      it 'requests Gitaly to remove the object pool' do
+        expect(Gitlab::GitalyClient).to receive(:call).with(pool.shard_name, :object_pool_service, :delete_object_pool, Object)
+
+        subject.perform(pool.id)
+      end
+
+      it 'destroys the pool' do
+        subject.perform(pool.id)
+
+        expect(PoolRepository.find_by_id(pool.id)).to be_nil
+      end
+    end
+  end
+end