Merge branch 'da-recalculates-the-checksum-for-projects-up-to-date' into 'master'

Geo - Recalculates the checksum for projects up to date

Closes #6773 and #6423

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

ee/app/services/geo/repository_verification_primary_service.rb

+module Geo
+  class RepositoryVerificationPrimaryService
+    include Gitlab::Geo::ProjectLogHelpers
+
+    def initialize(project)
+      @project = project
+    end
+
+    def execute
+      calculate_repository_checksum
+      calculate_wiki_checksum
+    end
+
+    private
+
+    attr_reader :project
+
+    def calculate_repository_checksum
+      calculate_checksum(:repository, project.repository)
+    end
+
+    def calculate_wiki_checksum
+      calculate_checksum(:wiki, project.wiki.repository)
+    end
+
+    def calculate_checksum(type, repository)
+      update_repository_state!(type, checksum: repository.checksum)
+    rescue Gitlab::Git::Repository::NoRepository, Gitlab::Git::Repository::InvalidRepository
+      update_repository_state!(type, checksum: Gitlab::Git::Repository::EMPTY_REPOSITORY_CHECKSUM)
+    rescue => e
+      log_error('Error calculating the repository checksum', e, type: type)
+      update_repository_state!(type, failure: e.message)
+    end
+
+    def update_repository_state!(type, checksum: nil, failure: nil)
+      repository_state.update!(
+        "#{type}_verification_checksum" => checksum,
+        "last_#{type}_verification_failure" => failure
+      )
+    end
+
+    def repository_state
+      @repository_state ||= project.repository_state || project.build_repository_state
+    end
+  end
+end

ee/app/services/geo/repository_verify_secondary_service.rb → ee/app/services/geo/repository_verification_secondary_service.rb

 module Geo
-  class RepositoryVerifySecondaryService
+  class RepositoryVerificationSecondaryService
     include Gitlab::Geo::ProjectLogHelpers
 
     def initialize(registry, type)

ee/app/workers/geo/repository_verification/primary/single_worker.rb

@@ -5,7 +5,6 @@ module Geo
         include ApplicationWorker
         include GeoQueue
         include ExclusiveLeaseGuard
-        include Gitlab::Geo::ProjectLogHelpers
 
         LEASE_TIMEOUT = 1.hour.to_i
 
@@ -18,41 +17,12 @@ module Geo
           return if project.nil? || project.pending_delete?
 
           try_obtain_lease do
-            calculate_repository_checksum if repository_state.repository_verification_checksum.nil?
-            calculate_wiki_checksum if repository_state.wiki_verification_checksum.nil?
+            Geo::RepositoryVerificationPrimaryService.new(project).execute
           end
         end
 
         private
 
-        def calculate_repository_checksum
-          calculate_checksum(:repository, project.repository)
-        end
-
-        def calculate_wiki_checksum
-          calculate_checksum(:wiki, project.wiki.repository)
-        end
-
-        def calculate_checksum(type, repository)
-          update_repository_state!(type, checksum: repository.checksum)
-        rescue Gitlab::Git::Repository::NoRepository, Gitlab::Git::Repository::InvalidRepository
-          update_repository_state!(type, checksum: Gitlab::Git::Repository::EMPTY_REPOSITORY_CHECKSUM)
-        rescue => e
-          log_error('Error calculating the repository checksum', e, type: type)
-          update_repository_state!(type, failure: e.message)
-        end
-
-        def update_repository_state!(type, checksum: nil, failure: nil)
-          repository_state.update!(
-            "#{type}_verification_checksum" => checksum,
-            "last_#{type}_verification_failure" => failure
-          )
-        end
-
-        def repository_state
-          @repository_state ||= project.repository_state || project.build_repository_state
-        end
-
         def lease_key
           "geo:single_repository_verification_worker:#{project.id}"
         end

ee/app/workers/geo/repository_verification/secondary/single_worker.rb

@@ -17,7 +17,7 @@ module Geo
         def perform(registry_id)
           return unless Gitlab::Geo.secondary?
 
-          @registry = Geo::ProjectRegistry.find_by_id(registry_id)
+          @registry = Geo::ProjectRegistry.find_by(id: registry_id)
           return if registry.nil? || project.nil? || project.pending_delete?
 
           try_obtain_lease do
@@ -29,7 +29,7 @@ module Geo
         private
 
         def verify_checksum(type)
-          Geo::RepositoryVerifySecondaryService.new(registry, type).execute
+          Geo::RepositoryVerificationSecondaryService.new(registry, type).execute
         rescue => e
           log_error('Error verifying the repository checksum', e, type: type)
           raise e

ee/changelogs/unreleased/da-recalculates-the-checksum-for-projects-up-to-date.yml

+---
+title: Geo - Recalculates the checksum for projects up to date
+merge_request: 6333
+author:
+type: fixed

ee/spec/services/geo/repository_verification_primary_service_spec.rb

+require 'spec_helper'
+
+describe Geo::RepositoryVerificationPrimaryService do
+  let(:project) { create(:project) }
+  let(:repository) { double(checksum: 'f123') }
+  let(:wiki) { double(checksum: 'e321') }
+
+  subject(:service) { described_class.new(project) }
+
+  describe '#perform' do
+    it 'calculates the checksum for unverified projects' do
+      stub_project_repository(project, repository)
+      stub_wiki_repository(project.wiki, wiki)
+
+      subject.execute
+
+      expect(project.repository_state).to have_attributes(
+        repository_verification_checksum: 'f123',
+        last_repository_verification_failure: nil,
+        wiki_verification_checksum: 'e321',
+        last_wiki_verification_failure: nil
+      )
+    end
+
+    it 'calculates the checksum for outdated projects' do
+      stub_project_repository(project, repository)
+      stub_wiki_repository(project.wiki, wiki)
+
+      repository_state =
+        create(:repository_state,
+          project: project,
+          repository_verification_checksum: nil,
+          wiki_verification_checksum: nil)
+
+      subject.execute
+
+      expect(repository_state.reload).to have_attributes(
+        repository_verification_checksum: 'f123',
+        last_repository_verification_failure: nil,
+        wiki_verification_checksum: 'e321',
+        last_wiki_verification_failure: nil
+      )
+    end
+
+    it 'calculates the checksum for outdated repositories/wikis' do
+      stub_project_repository(project, repository)
+      stub_wiki_repository(project.wiki, wiki)
+
+      repository_state =
+        create(:repository_state,
+          project: project,
+          repository_verification_checksum: nil,
+          wiki_verification_checksum: nil)
+
+      subject.execute
+
+      expect(repository_state.reload).to have_attributes(
+        repository_verification_checksum: 'f123',
+        last_repository_verification_failure: nil,
+        wiki_verification_checksum: 'e321',
+        last_wiki_verification_failure: nil
+      )
+    end
+
+    it 'recalculates the checksum for projects up to date' do
+      stub_project_repository(project, repository)
+      stub_wiki_repository(project.wiki, wiki)
+
+      create(:repository_state,
+        project: project,
+        repository_verification_checksum: 'f079a831cab27bcda7d81cd9b48296d0c3dd92ee',
+        wiki_verification_checksum: 'e079a831cab27bcda7d81cd9b48296d0c3dd92ef')
+
+      expect(repository).to receive(:checksum)
+      expect(wiki).to receive(:checksum)
+
+      subject.execute
+    end
+
+    it 'calculates the wiki checksum even when wiki is not enabled for project' do
+      stub_project_repository(project, repository)
+      stub_wiki_repository(project.wiki, wiki)
+
+      project.update!(wiki_enabled: false)
+
+      subject.execute
+
+      expect(project.repository_state).to have_attributes(
+        repository_verification_checksum: 'f123',
+        last_repository_verification_failure: nil,
+        wiki_verification_checksum: 'e321',
+        last_wiki_verification_failure: nil
+      )
+    end
+
+    it 'does not mark the calculating as failed when there is no repo' do
+      subject.execute
+
+      expect(project.repository_state).to have_attributes(
+        repository_verification_checksum: '0000000000000000000000000000000000000000',
+        last_repository_verification_failure: nil,
+        wiki_verification_checksum: '0000000000000000000000000000000000000000',
+        last_wiki_verification_failure: nil
+      )
+    end
+
+    it 'does not mark the calculating as failed for non-valid repo' do
+      project_broken_repo = create(:project, :broken_repo)
+
+      service = described_class.new(project_broken_repo)
+      service.execute
+
+      expect(project_broken_repo.repository_state).to have_attributes(
+        repository_verification_checksum: '0000000000000000000000000000000000000000',
+        last_repository_verification_failure: nil,
+        wiki_verification_checksum: '0000000000000000000000000000000000000000',
+        last_wiki_verification_failure: nil
+      )
+    end
+
+    it 'keeps track of failures when calculating the repository checksum' do
+      stub_project_repository(project, repository)
+      stub_wiki_repository(project.wiki, wiki)
+
+      allow(repository).to receive(:checksum).and_raise('Something went wrong with repository')
+      allow(wiki).to receive(:checksum).twice.and_raise('Something went wrong with wiki')
+
+      subject.execute
+
+      expect(project.repository_state).to have_attributes(
+        repository_verification_checksum: nil,
+        last_repository_verification_failure: 'Something went wrong with repository',
+        wiki_verification_checksum: nil,
+        last_wiki_verification_failure: 'Something went wrong with wiki'
+      )
+    end
+  end
+
+  def stub_project_repository(project, repository)
+    allow(Repository).to receive(:new).with(
+      project.full_path,
+      project,
+      disk_path: project.disk_path
+    ).and_return(repository)
+  end
+
+  def stub_wiki_repository(wiki, repository)
+    allow(Repository).to receive(:new).with(
+      project.wiki.full_path,
+      project,
+      disk_path: project.wiki.disk_path,
+      is_wiki: true
+    ).and_return(repository)
+  end
+end

ee/spec/services/geo/repository_verify_secondary_service_spec.rb → ee/spec/services/geo/repository_verification_secondary_service_spec.rb

 require 'spec_helper'
 
-describe Geo::RepositoryVerifySecondaryService, :geo do
+describe Geo::RepositoryVerificationSecondaryService, :geo do
   include ::EE::GeoHelpers
 
   let(:secondary) { create(:geo_node) }

ee/spec/workers/geo/repository_verification/primary/single_worker_spec.rb

@@ -7,202 +7,45 @@ describe Geo::RepositoryVerification::Primary::SingleWorker, :postgresql, :clean
   set(:project) { create(:project) }
 
   let!(:primary) { create(:geo_node, :primary) }
-  let(:repository) { double(checksum: 'f123') }
-  let(:wiki) { double(checksum: 'e321') }
 
   before do
     stub_current_geo_node(primary)
-    stub_exclusive_lease
   end
 
   describe '#perform' do
     it 'does not calculate the checksum when not running on a primary' do
-      stub_project_repository(project, repository)
-      stub_wiki_repository(project.wiki, wiki)
-
       allow(Gitlab::Geo).to receive(:primary?) { false }
 
-      expect(repository).not_to receive(:checksum)
-      expect(wiki).not_to receive(:checksum)
-
       subject.perform(project.id)
+
+      expect(project.reload.repository_state).to be_nil
     end
 
     it 'does not calculate the checksum when project is pending deletion' do
-      stub_project_repository(project, repository)
-      stub_wiki_repository(project.wiki, wiki)
-
       project.update!(pending_delete: true)
 
-      expect(repository).not_to receive(:checksum)
-      expect(wiki).not_to receive(:checksum)
-
       subject.perform(project.id)
+
+      expect(project.reload.repository_state).to be_nil
     end
 
     it 'does not raise an error when project could not be found' do
       expect { subject.perform(-1) }.not_to raise_error
     end
 
-    it 'calculates the checksum for unverified projects' do
-      stub_project_repository(project, repository)
-      stub_wiki_repository(project.wiki, wiki)
-
-      subject.perform(project.id)
-
-      expect(project.repository_state).to have_attributes(
-        repository_verification_checksum: 'f123',
-        last_repository_verification_failure: nil,
-        wiki_verification_checksum: 'e321',
-        last_wiki_verification_failure: nil
-      )
-    end
-
-    it 'calculates the checksum for outdated projects' do
-      stub_project_repository(project, repository)
-      stub_wiki_repository(project.wiki, wiki)
-
-      repository_state =
-        create(:repository_state,
-          project: project,
-          repository_verification_checksum: nil,
-          wiki_verification_checksum: nil)
-
-      subject.perform(project.id)
-
-      expect(repository_state.reload).to have_attributes(
-        repository_verification_checksum: 'f123',
-        last_repository_verification_failure: nil,
-        wiki_verification_checksum: 'e321',
-        last_wiki_verification_failure: nil
-      )
-    end
-
-    it 'calculates the checksum for outdated repositories' do
-      stub_project_repository(project, repository)
-
-      repository_state =
-        create(:repository_state,
-          project: project,
-          repository_verification_checksum: nil,
-          wiki_verification_checksum: 'e079a831cab27bcda7d81cd9b48296d0c3dd92ef')
-
-      subject.perform(project.id)
-
-      expect(repository_state.reload).to have_attributes(
-        repository_verification_checksum: 'f123',
-        last_repository_verification_failure: nil,
-        wiki_verification_checksum: 'e079a831cab27bcda7d81cd9b48296d0c3dd92ef',
-        last_wiki_verification_failure: nil
-      )
-    end
-
-    it 'calculates the checksum for outdated wikis' do
-      stub_wiki_repository(project.wiki, wiki)
-
-      repository_state =
-        create(:repository_state,
-          project: project,
-          repository_verification_checksum: 'f079a831cab27bcda7d81cd9b48296d0c3dd92ee',
-          wiki_verification_checksum: nil)
-
-      subject.perform(project.id)
-
-      expect(repository_state.reload).to have_attributes(
-        repository_verification_checksum: 'f079a831cab27bcda7d81cd9b48296d0c3dd92ee',
-        last_repository_verification_failure: nil,
-        wiki_verification_checksum: 'e321',
-        last_wiki_verification_failure: nil
-      )
-    end
-
-    it 'does not recalculate the checksum for projects up to date' do
-      stub_project_repository(project, repository)
-      stub_wiki_repository(project.wiki, wiki)
-
-      create(:repository_state,
-        project: project,
-        repository_verification_checksum: 'f079a831cab27bcda7d81cd9b48296d0c3dd92ee',
-        wiki_verification_checksum: 'e079a831cab27bcda7d81cd9b48296d0c3dd92ef')
-
-      expect(repository).not_to receive(:checksum)
-      expect(wiki).not_to receive(:checksum)
-
-      subject.perform(project.id)
-    end
-
-    it 'calculates the wiki checksum even when wiki is not enabled for project' do
-      stub_project_repository(project, repository)
-      stub_wiki_repository(project.wiki, wiki)
-
-      project.update!(wiki_enabled: false)
-
-      subject.perform(project.id)
-
-      expect(project.repository_state).to have_attributes(
-        repository_verification_checksum: 'f123',
-        last_repository_verification_failure: nil,
-        wiki_verification_checksum: 'e321',
-        last_wiki_verification_failure: nil
-      )
-    end
-
-    it 'does not mark the calculating as failed when there is no repo' do
-      subject.perform(project.id)
+    it 'delegates the checksum calculation to Geo::RepositoryVerificationPrimaryService' do
+      stub_exclusive_lease
 
-      expect(project.repository_state).to have_attributes(
-        repository_verification_checksum: '0000000000000000000000000000000000000000',
-        last_repository_verification_failure: nil,
-        wiki_verification_checksum: '0000000000000000000000000000000000000000',
-        last_wiki_verification_failure: nil
-      )
-    end
+      service = instance_double(Geo::RepositoryVerificationPrimaryService, execute: true)
 
-    it 'does not mark the calculating as failed for non-valid repo' do
-      project_broken_repo = create(:project, :broken_repo)
-
-      subject.perform(project_broken_repo.id)
-
-      expect(project_broken_repo.repository_state).to have_attributes(
-        repository_verification_checksum: '0000000000000000000000000000000000000000',
-        last_repository_verification_failure: nil,
-        wiki_verification_checksum: '0000000000000000000000000000000000000000',
-        last_wiki_verification_failure: nil
-      )
-    end
-
-    it 'keeps track of failures when calculating the repository checksum' do
-      stub_project_repository(project, repository)
-      stub_wiki_repository(project.wiki, wiki)
-
-      allow(repository).to receive(:checksum).and_raise('Something went wrong with repository')
-      allow(wiki).to receive(:checksum).twice.and_raise('Something went wrong with wiki')
+      allow(Geo::RepositoryVerificationPrimaryService)
+        .to receive(:new)
+        .with(project)
+        .and_return(service)
 
       subject.perform(project.id)
 
-      expect(project.repository_state).to have_attributes(
-        repository_verification_checksum: nil,
-        last_repository_verification_failure: 'Something went wrong with repository',
-        wiki_verification_checksum: nil,
-        last_wiki_verification_failure: 'Something went wrong with wiki'
-      )
+      expect(service).to have_received(:execute)
     end
   end
-
-  def stub_project_repository(project, repository)
-    allow(Repository).to receive(:new).with(
-      project.full_path,
-      project,
-      disk_path: project.disk_path
-    ).and_return(repository)
-  end
-
-  def stub_wiki_repository(wiki, repository)
-    allow(Repository).to receive(:new).with(
-      project.wiki.full_path,
-      project,
-      disk_path: project.wiki.disk_path,
-      is_wiki: true
-    ).and_return(repository)
-  end
 end

ee/spec/workers/geo/repository_verification/secondary/single_worker_spec.rb

@@ -10,24 +10,33 @@ describe Geo::RepositoryVerification::Secondary::SingleWorker, :postgresql, :cle
 
   before do
     stub_current_geo_node(secondary)
-    stub_exclusive_lease
   end
 
   describe '#perform' do
     it 'does not calculate the checksum when not running on a secondary' do
       allow(Gitlab::Geo).to receive(:secondary?) { false }
 
-      expect_any_instance_of(Geo::RepositoryVerifySecondaryService).not_to receive(:execute)
-
       subject.perform(registry.id)
+
+      expect(registry.reload).to have_attributes(
+        repository_verification_checksum_sha: nil,
+        last_repository_verification_failure: nil,
+        wiki_verification_checksum_sha: nil,
+        last_wiki_verification_failure: nil
+      )
     end
 
     it 'does not calculate the checksum when project is pending deletion' do
       registry.project.update!(pending_delete: true)
 
-      expect_any_instance_of(Geo::RepositoryVerifySecondaryService).not_to receive(:execute)
-
       subject.perform(registry.id)
+
+      expect(registry.reload).to have_attributes(
+        repository_verification_checksum_sha: nil,
+        last_repository_verification_failure: nil,
+        wiki_verification_checksum_sha: nil,
+        last_wiki_verification_failure: nil
+      )
     end
 
     it 'does not raise an error when registry could not be found' do
@@ -41,12 +50,24 @@ describe Geo::RepositoryVerification::Secondary::SingleWorker, :postgresql, :cle
     end
 
     it 'runs verification for both repository and wiki' do
-      create(:repository_state, project: project, repository_verification_checksum: 'my_checksum', wiki_verification_checksum: 'my_checksum')
+      stub_exclusive_lease
 
-      expect(Geo::RepositoryVerifySecondaryService).to receive(:new).with(registry, :repository).and_call_original
-      expect(Geo::RepositoryVerifySecondaryService).to receive(:new).with(registry, :wiki).and_call_original
+      service =
+        instance_double(Geo::RepositoryVerificationSecondaryService, execute: true)
+
+      allow(Geo::RepositoryVerificationSecondaryService)
+        .to receive(:new)
+        .with(registry, :repository)
+        .and_return(service)
+
+      allow(Geo::RepositoryVerificationSecondaryService)
+        .to receive(:new)
+        .with(registry, :wiki)
+        .and_return(service)
 
       subject.perform(registry.id)
+
+      expect(service).to have_received(:execute).twice
     end
   end
 end