Merge branch 'mk/geo-replicate-keep-around-refs' into 'master'

Geo: Replicate keep around refs

Closes #7245

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

app/models/ci/pipeline.rb

@@ -666,8 +666,7 @@ module Ci
     def keep_around_commits
       return unless project
 
-      project.repository.keep_around(self.sha)
-      project.repository.keep_around(self.before_sha)
+      project.repository.keep_around(self.sha, self.before_sha)
     end
 
     def valid_source

app/models/diff_note.rb

@@ -191,14 +191,18 @@ class DiffNote < Note
   end
 
   def keep_around_commits
-    project.repository.keep_around(self.original_position.base_sha)
-    project.repository.keep_around(self.original_position.start_sha)
-    project.repository.keep_around(self.original_position.head_sha)
+    shas = [
+      self.original_position.base_sha,
+      self.original_position.start_sha,
+      self.original_position.head_sha
+    ]
 
     if self.position != self.original_position
-      project.repository.keep_around(self.position.base_sha)
-      project.repository.keep_around(self.position.start_sha)
-      project.repository.keep_around(self.position.head_sha)
+      shas << self.position.base_sha
+      shas << self.position.start_sha
+      shas << self.position.head_sha
     end
+
+    project.repository.keep_around(*shas)
   end
 end

app/models/merge_request_diff.rb

@@ -314,9 +314,7 @@ class MergeRequestDiff < ActiveRecord::Base
 
   def keep_around_commits
     [repository, merge_request.source_project.repository].uniq.each do |repo|
-      repo.keep_around(start_commit_sha)
-      repo.keep_around(head_commit_sha)
-      repo.keep_around(base_commit_sha)
+      repo.keep_around(start_commit_sha, head_commit_sha, base_commit_sha)
     end
   end
 end

app/models/repository.rb

@@ -252,15 +252,22 @@ class Repository
   # Git GC will delete commits from the repository that are no longer in any
   # branches or tags, but we want to keep some of these commits around, for
   # example if they have comments or CI builds.
-  def keep_around(sha)
-    return unless sha.present? && commit_by(oid: sha)
+  #
+  # For Geo's sake, pass in multiple shas rather than calling it multiple times,
+  # to avoid unnecessary syncing.
+  def keep_around(*shas)
+    shas.each do |sha|
+      begin
+        next unless sha.present? && commit_by(oid: sha)
 
-    return if kept_around?(sha)
+        next if kept_around?(sha)
 
-    # This will still fail if the file is corrupted (e.g. 0 bytes)
-    raw_repository.write_ref(keep_around_ref_name(sha), sha, shell: false)
-  rescue Gitlab::Git::CommandError => ex
-    Rails.logger.error "Unable to create keep-around reference for repository #{disk_path}: #{ex}"
+        # This will still fail if the file is corrupted (e.g. 0 bytes)
+        raw_repository.write_ref(keep_around_ref_name(sha), sha, shell: false)
+      rescue Gitlab::Git::CommandError => ex
+        Rails.logger.error "Unable to create keep-around reference for repository #{disk_path}: #{ex}"
+      end
+    end
   end
 
   def kept_around?(sha)

ee/app/models/ee/project.rb

@@ -520,7 +520,8 @@ module EE
     override :after_import
     def after_import
       super
-      log_geo_events
+      repository.log_geo_updated_event
+      wiki.repository.log_geo_updated_event
     end
 
     override :import?
@@ -573,13 +574,6 @@ module EE
       # Board limits are disabled in EE, so this method is just a no-op.
     end
 
-    def log_geo_events
-      return unless ::Gitlab::Geo.primary?
-
-      ::Geo::RepositoryUpdatedService.new(self, source: ::Geo::RepositoryUpdatedEvent::REPOSITORY).execute
-      ::Geo::RepositoryUpdatedService.new(self, source: ::Geo::RepositoryUpdatedEvent::WIKI).execute
-    end
-
     override :after_rename_repository
     def after_rename_repository(full_path_before, path_before)
       super(full_path_before, path_before)

ee/app/models/ee/repository.rb

@@ -5,6 +5,7 @@ module EE
   # and be prepended in the `Repository` model
   module Repository
     extend ActiveSupport::Concern
+    extend ::Gitlab::Utils::Override
 
     MIRROR_REMOTE = "upstream".freeze
 
@@ -69,5 +70,22 @@ module EE
         false
       end
     end
+
+    override :keep_around
+    def keep_around(*shas)
+      super
+    ensure
+      log_geo_updated_event
+    end
+
+    def log_geo_updated_event
+      return unless ::Gitlab::Geo.primary?
+
+      ::Geo::RepositoryUpdatedService.new(self).execute
+    end
+
+    def geo_updated_event_source
+      is_wiki ? Geo::RepositoryUpdatedEvent::WIKI : Geo::RepositoryUpdatedEvent::REPOSITORY
+    end
   end
 end

ee/app/services/ee/projects/update_service.rb

@@ -63,7 +63,7 @@ module EE
       end
 
       def sync_wiki_on_enable
-        ::Geo::RepositoryUpdatedService.new(project, source: ::Geo::RepositoryUpdatedEvent::WIKI).execute
+        ::Geo::RepositoryUpdatedService.new(project.wiki.repository).execute
       end
     end
   end

ee/app/services/ee/wiki_pages/base_service.rb

@@ -16,7 +16,7 @@ module EE
 
       def process_wiki_repository_update
         if ::Gitlab::Geo.primary?
-          ::Geo::RepositoryUpdatedService.new(project, source: ::Geo::RepositoryUpdatedEvent::WIKI).execute
+          ::Geo::RepositoryUpdatedService.new(project.wiki.repository).execute
         end
       end
     end

ee/app/services/geo/repository_updated_service.rb

@@ -4,12 +4,12 @@ module Geo
 
     RepositoryUpdateError = Class.new(StandardError)
 
-    def initialize(project, params = {})
-      @project = project
+    def initialize(repository, params = {})
+      @project = repository.project
       @params  = params
       @refs    = params.fetch(:refs, [])
       @changes = params.fetch(:changes, [])
-      @source  = params.fetch(:source, Geo::RepositoryUpdatedEvent::REPOSITORY)
+      @source  = repository.geo_updated_event_source
     end
 
     def execute

ee/app/workers/ee/post_receive.rb

@@ -12,7 +12,7 @@ module EE
       super
 
       if ::Gitlab::Geo.primary?
-        ::Geo::RepositoryUpdatedService.new(post_received.project, refs: refs, changes: changes).execute
+        ::Geo::RepositoryUpdatedService.new(post_received.project.repository, refs: refs, changes: changes).execute
       end
     end
 
@@ -22,7 +22,7 @@ module EE
       update_wiki_es_indexes(post_received)
 
       if ::Gitlab::Geo.primary?
-        ::Geo::RepositoryUpdatedService.new(post_received.project, source: ::Geo::RepositoryUpdatedEvent::WIKI).execute
+        ::Geo::RepositoryUpdatedService.new(post_received.project.wiki.repository).execute
       end
     end
 

ee/changelogs/unreleased/mk-geo-replicate-keep-around-refs.yml

+---
+title: 'Geo: Replicate keep around refs'
+merge_request: 6922
+author:
+type: fixed

ee/spec/models/project_spec.rb

@@ -1698,12 +1698,12 @@ describe Project do
     before do
       allow(::Geo::RepositoryUpdatedService)
         .to receive(:new)
-        .with(project, source: Geo::RepositoryUpdatedEvent::REPOSITORY)
+        .with(project.repository)
         .and_return(repository_updated_service)
 
       allow(::Geo::RepositoryUpdatedService)
         .to receive(:new)
-        .with(project, source: Geo::RepositoryUpdatedEvent::WIKI)
+        .with(project.wiki.repository)
         .and_return(wiki_updated_service)
     end
 

ee/spec/models/repository_spec.rb

@@ -2,6 +2,7 @@ require 'spec_helper'
 
 describe Repository do
   include RepoHelpers
+  include ::EE::GeoHelpers
   TestBlob = Struct.new(:path)
 
   let(:project) { create(:project, :repository) }
@@ -113,4 +114,44 @@ describe Repository do
       expect(repository.upstream_branches.first.name).to eq('upstream_branch')
     end
   end
+
+  describe '#keep_around' do
+    set(:primary_node)   { create(:geo_node, :primary) }
+    set(:secondary_node) { create(:geo_node) }
+    let(:sha) { sample_commit.id }
+
+    context 'on a Geo primary' do
+      before do
+        stub_current_geo_node(primary_node)
+      end
+
+      context 'when a single SHA is passed' do
+        it 'creates a RepositoryUpdatedEvent' do
+          expect do
+            repository.keep_around(sha)
+          end.to change { ::Geo::RepositoryUpdatedEvent.count }.by(1)
+        end
+      end
+
+      context 'when multiple SHAs are passed' do
+        it 'creates exactly one RepositoryUpdatedEvent' do
+          expect do
+            repository.keep_around(sha, sample_big_commit.id)
+          end.to change { ::Geo::RepositoryUpdatedEvent.count }.by(1)
+        end
+      end
+    end
+
+    context 'on a Geo secondary' do
+      before do
+        stub_current_geo_node(secondary_node)
+      end
+
+      it 'does not create a RepositoryUpdatedEvent' do
+        expect do
+          repository.keep_around(sha)
+        end.not_to change { ::Geo::RepositoryUpdatedEvent.count }
+      end
+    end
+  end
 end

ee/spec/services/geo/repository_updated_service_spec.rb

@@ -13,7 +13,7 @@ describe Geo::RepositoryUpdatedService do
   end
 
   describe '#execute' do
-    subject { described_class.new(project, source: source) }
+    subject { described_class.new(repository) }
 
     shared_examples 'repository being updated' do
       context 'when not running on a primary node' do
@@ -59,7 +59,7 @@ describe Geo::RepositoryUpdatedService do
         end
 
         it 'does not raise an error when project have never been verified' do
-          expect { described_class.new(create(:project)) }.not_to raise_error
+          expect { described_class.new(create(:project).repository) }.not_to raise_error
         end
 
         it 'raises a Geo::RepositoryUpdatedService::RepositoryUpdateError when an error occurs' do
@@ -74,14 +74,14 @@ describe Geo::RepositoryUpdatedService do
 
     context 'when repository is being updated' do
       include_examples 'repository being updated' do
-        let(:source) { Geo::RepositoryUpdatedEvent::REPOSITORY }
+        let(:repository) { project.repository }
         let(:method_prefix) { 'repository' }
       end
     end
 
     context 'when wiki is being updated' do
       include_examples 'repository being updated' do
-        let(:source) { Geo::RepositoryUpdatedEvent::WIKI }
+        let(:repository) { project.wiki.repository }
         let(:method_prefix) { 'wiki' }
       end
     end

ee/spec/services/wiki_pages/create_service_spec.rb

@@ -23,7 +23,7 @@ describe WikiPages::CreateService do
       allow(Gitlab::Geo).to receive(:primary?) { true }
 
       repository_updated_service = instance_double('::Geo::RepositoryUpdatedService')
-      expect(::Geo::RepositoryUpdatedService).to receive(:new).with(project, source: Geo::RepositoryUpdatedEvent::WIKI) { repository_updated_service }
+      expect(::Geo::RepositoryUpdatedService).to receive(:new).with(project.wiki.repository) { repository_updated_service }
       expect(repository_updated_service).to receive(:execute)
 
       service.execute
@@ -32,7 +32,7 @@ describe WikiPages::CreateService do
     it 'does not call Geo::RepositoryUpdatedService when not running on a Geo primary node' do
       allow(Gitlab::Geo).to receive(:primary?) { false }
 
-      expect(::Geo::RepositoryUpdatedService).not_to receive(:new).with(project, source: Geo::RepositoryUpdatedEvent::WIKI)
+      expect(::Geo::RepositoryUpdatedService).not_to receive(:new).with(project.wiki.repository)
 
       service.execute
     end

ee/spec/services/wiki_pages/destroy_service_spec.rb

@@ -16,7 +16,7 @@ describe WikiPages::DestroyService do
       allow(Gitlab::Geo).to receive(:primary?) { true }
 
       repository_updated_service = instance_double('::Geo::RepositoryUpdatedService')
-      expect(::Geo::RepositoryUpdatedService).to receive(:new).with(project, source: Geo::RepositoryUpdatedEvent::WIKI) { repository_updated_service }
+      expect(::Geo::RepositoryUpdatedService).to receive(:new).with(project.wiki.repository) { repository_updated_service }
       expect(repository_updated_service).to receive(:execute)
 
       service.execute(page)
@@ -25,7 +25,7 @@ describe WikiPages::DestroyService do
     it 'does not call Geo::RepositoryUpdatedService when not running on a Geo primary node' do
       allow(Gitlab::Geo).to receive(:primary?) { false }
 
-      expect(::Geo::RepositoryUpdatedService).not_to receive(:new).with(project, source: Geo::RepositoryUpdatedEvent::WIKI)
+      expect(::Geo::RepositoryUpdatedService).not_to receive(:new).with(project.wiki.repository)
 
       service.execute(page)
     end

ee/spec/services/wiki_pages/update_service_spec.rb

@@ -24,7 +24,7 @@ describe WikiPages::UpdateService do
       allow(Gitlab::Geo).to receive(:primary?) { true }
 
       repository_updated_service = instance_double('::Geo::RepositoryUpdatedService')
-      expect(::Geo::RepositoryUpdatedService).to receive(:new).with(project, source: Geo::RepositoryUpdatedEvent::WIKI) { repository_updated_service }
+      expect(::Geo::RepositoryUpdatedService).to receive(:new).with(project.wiki.repository) { repository_updated_service }
       expect(repository_updated_service).to receive(:execute)
 
       service.execute(page)
@@ -33,7 +33,7 @@ describe WikiPages::UpdateService do
     it 'does not call Geo::RepositoryUpdatedService when not running on a Geo primary node' do
       allow(Gitlab::Geo).to receive(:primary?) { false }
 
-      expect(::Geo::RepositoryUpdatedService).not_to receive(:new).with(project, source: Geo::RepositoryUpdatedEvent::WIKI)
+      expect(::Geo::RepositoryUpdatedService).not_to receive(:new).with(project.wiki.repository)
 
       service.execute(page)
     end

spec/models/repository_spec.rb

@@ -2005,6 +2005,24 @@ describe Repository do
 
       File.delete(path)
     end
+
+    context 'for multiple SHAs' do
+      it 'skips non-existent SHAs' do
+        repository.keep_around('aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa', sample_commit.id)
+
+        expect(repository.kept_around?(sample_commit.id)).to be_truthy
+      end
+
+      it 'skips already-kept-around SHAs' do
+        repository.keep_around(sample_commit.id)
+
+        expect(repository.raw_repository).to receive(:write_ref).exactly(1).and_call_original
+
+        repository.keep_around(sample_commit.id, another_sample_commit.id)
+
+        expect(repository.kept_around?(another_sample_commit.id)).to be_truthy
+      end
+    end
   end
 
   describe '#update_ref' do