Merge branch 'da-ensure-repository-deletions-are-communicated-to-secondary' into 'master'

Geo: Ensure that repository deletions are communicated to the secondary

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

app/models/geo/deleted_project.rb

-class Geo::DeletedProject < ::Project
-  after_initialize :readonly!
+class Geo::DeletedProject
+  include Gitlab::CurrentSettings
 
-  def initialize(id:, name:, full_path:, repository_storage:)
-    repository_storage ||= current_application_settings.pick_repository_storage
+  attr_reader :id, :name, :disk_path
 
-    super(id: id, name: name, repository_storage: repository_storage)
-    @full_path = full_path
+  def initialize(id:, name:, disk_path:, repository_storage:)
+    @id = id
+    @name = name
+    @disk_path = disk_path
+    @repository_storage = repository_storage
   end
 
-  def full_path
-    @full_path
+  alias_method :full_path, :disk_path
+
+  def repository
+    @repository ||= Repository.new(disk_path, self)
+  end
+
+  def repository_storage
+    @repository_storage ||= current_application_settings.pick_repository_storage
+  end
+
+  def repository_storage_path
+    Gitlab.config.repositories.storages[repository_storage].try(:[], 'path')
+  end
+
+  def wiki
+    @wiki ||= ProjectWiki.new(self, nil)
+  end
+
+  def wiki_path
+    wiki.disk_path
+  end
+
+  # When we remove project we move the repository to path+deleted.git then
+  # outside the transaction we schedule removal of path+deleted with Sidekiq
+  # through the run_after_commit callback. In a Geo secondary node, we don't
+  # attempt to remove the repositories inside a transaction because we don't
+  # have access to the original model anymore, we just need to perform some
+  # cleanup. This method will run the given block to remove repositories
+  # immediately otherwise will leave us with stalled repositories on disk.
+  def run_after_commit(&block)
+    instance_eval(&block)
   end
-  alias_method :path_with_namespace, :full_path
 end

app/services/geo/repository_destroy_service.rb

 module Geo
   class RepositoryDestroyService
-    attr_reader :id, :name, :full_path, :storage_name
+    attr_reader :id, :name, :disk_path, :repository_storage
 
-    def initialize(id, name, full_path, storage_name)
+    def initialize(id, name, disk_path, repository_storage)
       @id = id
       @name = name
-      @full_path = full_path
-      @storage_name = storage_name
+      @disk_path = disk_path
+      @repository_storage = repository_storage
     end
 
     def async_execute
-      GeoRepositoryDestroyWorker.perform_async(id, name, full_path, storage_name)
+      GeoRepositoryDestroyWorker.perform_async(id, name, disk_path, repository_storage)
     end
 
     def execute
@@ -24,8 +24,8 @@ module Geo
       # rebuilding only what our service class requires
       ::Geo::DeletedProject.new(id: id,
                                 name: name,
-                                full_path: full_path,
-                                repository_storage: storage_name)
+                                disk_path: disk_path,
+                                repository_storage: repository_storage)
     end
   end
 end

app/workers/geo_repository_destroy_worker.rb

@@ -3,7 +3,7 @@ class GeoRepositoryDestroyWorker
   include GeoQueue
   include Gitlab::ShellAdapter
 
-  def perform(id, name, full_path, storage_name)
-    Geo::RepositoryDestroyService.new(id, name, full_path, storage_name).execute
+  def perform(id, name, disk_path, storage_name)
+    Geo::RepositoryDestroyService.new(id, name, disk_path, storage_name).execute
   end
 end

changelogs/unreleased-ee/da-ensure-repository-deletions-are-communicated-to-secondary.yml

+---
+title: Geo - Ensure that repository deletions in a primary node are correctly deleted
+  in a secondary node
+merge_request:
+author:
+type: fixed

ee/app/services/ee/projects/destroy_service.rb

@@ -22,29 +22,27 @@ module EE
       end
 
       def log_geo_event(project)
-        ::Geo::RepositoryDeletedEventStore.new(project,
+        ::Geo::RepositoryDeletedEventStore.new(
+          project,
           repo_path: repo_path,
-          wiki_path: wiki_path).create
+          wiki_path: wiki_path
+        ).create
       end
 
       # Removes physical repository in a Geo replicated secondary node
       # There is no need to do any database operation as it will be
       # replicated by itself.
       def geo_replicate
+        return unless ::Gitlab::Geo.secondary?
+
         # Flush the cache for both repositories. This has to be done _before_
         # removing the physical repositories as some expiration code depends on
         # Git data (e.g. a list of branch names).
         flush_caches(project)
 
         trash_repositories!
-        remove_tracking_entries!
-        log_info("Project \"#{project.name}\" was removed")
-      end
-
-      def remove_tracking_entries!
-        return unless ::Gitlab::Geo.secondary?
 
-        ::Geo::ProjectRegistry.where(project_id: project.id).delete_all
+        log_info("Project \"#{project.name}\" was removed")
       end
 
       private

lib/gitlab/geo/log_cursor/daemon.rb

@@ -123,17 +123,16 @@ module Gitlab
         def handle_repository_deleted(event_log)
           event = event_log.repository_deleted_event
 
-          disk_path = File.join(event.repository_storage_path, event.deleted_path)
-
           job_id = ::Geo::RepositoryDestroyService
-                     .new(event.project_id, event.deleted_project_name, disk_path, event.repository_storage_name)
+                     .new(event.project_id, event.deleted_project_name, event.deleted_path, event.repository_storage_name)
                      .async_execute
 
           logger.event_info(
             event_log.created_at,
             message: 'Deleted project',
             project_id: event.project_id,
-            disk_path: disk_path,
+            repository_storage_name: event.repository_storage_name,
+            disk_path: event.deleted_path,
             job_id: job_id)
 
           # No need to create a project entry if it doesn't exist

spec/lib/gitlab/geo/log_cursor/daemon_spec.rb

@@ -139,25 +139,30 @@ describe Gitlab::Geo::LogCursor::Daemon, :postgresql, :clean_gitlab_redis_shared
 
     context 'when replaying a repository deleted event' do
       let(:event_log) { create(:geo_event_log, :deleted_event) }
-      let(:project) { event_log.repository_deleted_event.project }
       let!(:event_log_state) { create(:geo_event_log_state, event_id: event_log.id - 1) }
       let(:repository_deleted_event) { event_log.repository_deleted_event }
+      let(:project) { repository_deleted_event.project }
 
-      it 'does not create a new project registry' do
+      it 'does not create a tracking database entry' do
         expect { daemon.run_once! }.not_to change(Geo::ProjectRegistry, :count)
       end
 
       it 'schedules a GeoRepositoryDestroyWorker' do
         project_id   = repository_deleted_event.project_id
         project_name = repository_deleted_event.deleted_project_name
-        full_path    = File.join(repository_deleted_event.repository_storage_path,
-                                 repository_deleted_event.deleted_path)
+        project_path = repository_deleted_event.deleted_path
 
         expect(::GeoRepositoryDestroyWorker).to receive(:perform_async)
-          .with(project_id, project_name, full_path, project.repository_storage)
+          .with(project_id, project_name, project_path, project.repository_storage)
 
         daemon.run_once!
       end
+
+      it 'removes the tracking database entry if exist' do
+        create(:geo_project_registry, :synced, project: project)
+
+        expect { daemon.run_once! }.to change(Geo::ProjectRegistry, :count).by(-1)
+      end
     end
 
     context 'when replaying a repositories changed event' do

spec/models/geo/deleted_project_spec.rb

 require 'spec_helper'
 
 RSpec.describe Geo::DeletedProject, type: :model do
-  subject { described_class.new(id: 1, name: 'sample', full_path: 'root/sample', repository_storage: nil) }
+  include StubConfiguration
 
-  it { expect(subject).to be_kind_of(Project) }
+  before do
+    storages = {
+      'foo' => { 'path' => 'tmp/tests/storage_foo' },
+      'bar' => { 'path' => 'tmp/tests/storage_bar' }
+    }
 
-  describe '#full_path' do
-    it 'returns the initialized value' do
-      expect(subject.full_path).to eq 'root/sample'
-    end
+    stub_storage_settings(storages)
   end
 
-  describe '#path_with_namespace' do
-    it 'is an alias for full_path' do
-      full_path = described_class.instance_method(:full_path)
-      path_with_namespace = described_class.instance_method(:path_with_namespace)
+  subject { described_class.new(id: 1, name: 'sample', disk_path: 'root/sample', repository_storage: 'foo') }
+
+  it { is_expected.to respond_to(:id) }
+  it { is_expected.to respond_to(:name) }
+  it { is_expected.to respond_to(:disk_path) }
 
-      expect(path_with_namespace).to eq(full_path)
+  describe '#full_path' do
+    it 'is an alias for disk_path' do
+      expect(subject.full_path).to eq 'root/sample'
     end
   end
 
   describe '#repository' do
     it 'returns a valid repository' do
       expect(subject.repository).to be_kind_of(Repository)
-      expect(subject.repository.full_path).to eq('root/sample')
+      expect(subject.repository.disk_path).to eq('root/sample')
+    end
+  end
+
+  describe '#repository_storage' do
+    it 'returns the initialized value when set' do
+      expect(subject.repository_storage).to eq 'foo'
+    end
+
+    it 'picks storage from ApplicationSetting when value is not initialized' do
+      allow_any_instance_of(ApplicationSetting).to receive(:pick_repository_storage).and_return('bar')
+
+      subject = described_class.new(id: 1, name: 'sample', disk_path: 'root/sample', repository_storage: nil)
+
+      expect(subject.repository_storage).to eq('bar')
+    end
+  end
+
+  describe '#repository_storage_path' do
+    it 'returns the repository storage path' do
+      expect(subject.repository_storage_path).to eq('tmp/tests/storage_foo')
+    end
+  end
+
+  describe '#wiki' do
+    it 'returns a valid wiki repository' do
+      expect(subject.wiki).to be_kind_of(ProjectWiki)
+      expect(subject.wiki.disk_path).to eq('root/sample.wiki')
+    end
+  end
+
+  describe '#wiki_path' do
+    it 'returns the wiki repository path on disk' do
+      expect(subject.wiki_path).to eq('root/sample.wiki')
+    end
+  end
+
+  describe '#run_after_commit' do
+    it 'runs the given block changing self to the caller' do
+      expect(subject).to receive(:repository_storage_path).once
+
+      subject.run_after_commit { self.repository_storage_path }
     end
   end
 end

spec/services/geo/repository_destroy_service_spec.rb

 require 'spec_helper'
 
 describe Geo::RepositoryDestroyService do
+  include ::EE::GeoHelpers
+
+  set(:secondary) { create(:geo_node) }
   let(:project) { create(:project_empty_repo) }
+
   subject(:service) { described_class.new(project.id, project.name, project.disk_path, project.repository_storage) }
 
+  before do
+    stub_current_geo_node(secondary)
+  end
+
   describe '#async_execute' do
     it 'starts the worker' do
       expect(GeoRepositoryDestroyWorker).to receive(:perform_async)
@@ -29,6 +37,16 @@ describe Geo::RepositoryDestroyService do
 
         expect(project.gitlab_shell.exists?(project.repository_storage_path, "#{project.disk_path}.git")).to be_falsey
       end
+
+      it 'cleans up deleted repositories' do
+        project.delete
+
+        expect(::GitlabShellWorker).to receive(:perform_in)
+          .with(5.minutes, :remove_repository, project.repository_storage_path, "#{project.disk_path}+#{project.id}+deleted")
+          .and_return(true)
+
+        service.execute
+      end
     end
 
     context 'hashed storage project' do
@@ -43,6 +61,16 @@ describe Geo::RepositoryDestroyService do
 
         expect(project.gitlab_shell.exists?(project.repository_storage_path, "#{project.disk_path}.git")).to be_falsey
       end
+
+      it 'cleans up deleted repositories' do
+        project.delete
+
+        expect(::GitlabShellWorker).to receive(:perform_in)
+          .with(5.minutes, :remove_repository, project.repository_storage_path, "#{project.disk_path}+#{project.id}+deleted")
+          .and_return(true)
+
+        service.execute
+      end
     end
   end
 end