Merge branch 'da-gitaly-calculate-repository-checksum-ee' into 'master'

Repository checksum calculation is handled by Gitaly when feature is enabled

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

GITALY_SERVER_VERSION

-0.93.0
+0.94.0

Gemfile

@@ -436,7 +436,7 @@ group :ed25519 do
 end
 
 # Gitaly GRPC client
-gem 'gitaly-proto', '~> 0.91.0', require: 'gitaly'
+gem 'gitaly-proto', '~> 0.94.0', require: 'gitaly'
 gem 'grpc', '~> 1.10.0'
 
 # Locked until https://github.com/google/protobuf/issues/4210 is closed

Gemfile.lock

@@ -314,7 +314,7 @@ GEM
       po_to_json (>= 1.0.0)
       rails (>= 3.2.0)
     gherkin-ruby (0.3.2)
-    gitaly-proto (0.91.0)
+    gitaly-proto (0.94.0)
       google-protobuf (~> 3.1)
       grpc (~> 1.0)
     github-linguist (5.3.3)
@@ -1095,7 +1095,7 @@ DEPENDENCIES
   gettext (~> 3.2.2)
   gettext_i18n_rails (~> 1.8.0)
   gettext_i18n_rails_js (~> 1.3)
-  gitaly-proto (~> 0.91.0)
+  gitaly-proto (~> 0.94.0)
   github-linguist (~> 5.3.3)
   gitlab-flowdock-git-hook (~> 1.0.1)
   gitlab-license (~> 1.0)

changelogs/unreleased/da-gitaly-calculate-repository-checksum.yml

+---
+title: Repository checksum calculation is handled by Gitaly when feature is enabled
+merge_request:
+author:
+type: changed

ee/app/models/ee/repository.rb

@@ -6,6 +6,10 @@ module EE
   module Repository
     extend ActiveSupport::Concern
 
+    included do
+      delegate :checksum, to: :raw_repository
+    end
+
     # Transiently sets a configuration variable
     def with_config(values = {})
       values.each { |k, v| rugged.config[k] = v }

ee/app/services/geo/repository_updated_service.rb

@@ -2,6 +2,8 @@ module Geo
   class RepositoryUpdatedService
     include ::Gitlab::Geo::ProjectLogHelpers
 
+    RepositoryUpdateError = Class.new(StandardError)
+
     def initialize(project, params = {})
       @project = project
       @params  = params
@@ -37,7 +39,7 @@ module Geo
       repository_state.update!("#{repository_checksum_column}" => nil, "#{repository_failure_column}" => nil)
     rescue => e
       log_error('Cannot reset repository checksum', e)
-      raise Gitlab::Git::Checksum::Failure, "Cannot reset repository checksum: #{e}"
+      raise RepositoryUpdateError, "Cannot reset repository checksum: #{e}"
     end
 
     def repository_checksum_column

ee/app/services/geo/repository_verify_secondary_service.rb

@@ -40,14 +40,14 @@ module Geo
     end
 
     def verify_checksum
-      checksum = calculate_checksum(project.repository_storage, repository_path)
+      checksum = project.repository.checksum
 
       if mismatch?(checksum)
         update_registry!(failure: "#{type.to_s.capitalize} checksum mismatch: #{repository_path}")
       else
         update_registry!(checksum: checksum)
       end
-    rescue ::Gitlab::Git::Repository::NoRepository, ::Gitlab::Git::Checksum::Failure, Timeout::Error => e
+    rescue ::Gitlab::Git::Repository::NoRepository, ::Gitlab::Git::Repository::ChecksumError, Timeout::Error => e
       update_registry!(failure: "Error verifying #{type.to_s.capitalize} checksum: #{repository_path}", exception: e)
     end
 
@@ -55,10 +55,6 @@ module Geo
       primary_checksum != checksum
     end
 
-    def calculate_checksum(storage, relative_path)
-      Gitlab::Git::Checksum.new(storage, relative_path).calculate
-    end
-
     def update_registry!(checksum: nil, failure: nil, exception: nil, details: {})
       attrs = {
         "#{type}_verification_checksum_sha" => checksum,

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

@@ -26,16 +26,15 @@ module Geo
         private
 
         def calculate_repository_checksum
-          calculate_checksum(:repository, project.disk_path)
+          calculate_checksum(:repository, project.repository)
         end
 
         def calculate_wiki_checksum
-          calculate_checksum(:wiki, project.wiki.disk_path)
+          calculate_checksum(:wiki, project.wiki.repository)
         end
 
-        def calculate_checksum(type, repository_relative_path)
-          checksum = Gitlab::Git::Checksum.new(project.repository_storage, repository_relative_path)
-          update_repository_state!(type, checksum: checksum.calculate)
+        def calculate_checksum(type, repository)
+          update_repository_state!(type, checksum: repository.checksum)
         rescue => e
           log_error('Error calculating the repository checksum', e, type: type)
           update_repository_state!(type, failure: e.message)

ee/spec/services/geo/repository_updated_service_spec.rb

@@ -52,12 +52,12 @@ describe Geo::RepositoryUpdatedService do
           expect { described_class.new(create(:project)) }.not_to raise_error
         end
 
-        it 'raises a Gitlab::Git::Checksum error when an error occurs' do
+        it 'raises a Geo::RepositoryUpdatedService::RepositoryUpdateError when an error occurs' do
           allow(subject.repository_state).to receive(:update!)
             .with("#{method_prefix}_verification_checksum" => nil, "last_#{method_prefix}_verification_failure" => nil)
             .and_raise(ActiveRecord::RecordInvalid.new(repository_state))
 
-          expect { subject.execute }.to raise_error Gitlab::Git::Checksum::Failure, /Cannot reset repository checksum/
+          expect { subject.execute }.to raise_error Geo::RepositoryUpdatedService::RepositoryUpdateError, /Cannot reset repository checksum/
         end
       end
     end

ee/spec/services/geo/repository_verify_secondary_service_spec.rb

@@ -10,16 +10,12 @@ describe Geo::RepositoryVerifySecondaryService, :geo do
   end
 
   shared_examples 'verify checksums for repositories/wikis' do |type|
-    let(:checksum) { instance_double('Gitlab::Git::Checksum') }
-    let(:storage) { project.repository_storage }
-    let(:relative_path) { service.send(:repository_path) }
-
     subject(:service)  { described_class.new(registry, type) }
 
     it 'does not calculate the checksum when not running on a secondary' do
       allow(Gitlab::Geo).to receive(:secondary?) { false }
 
-      expect(Gitlab::Git::Checksum).not_to receive(:new).with(storage, relative_path)
+      expect(registry.project.repository).not_to receive(:checksum)
 
       service.execute
     end
@@ -27,7 +23,7 @@ describe Geo::RepositoryVerifySecondaryService, :geo do
     it 'does not verify the checksum if resync is needed' do
       registry.assign_attributes("resync_#{type}" => true)
 
-      expect(Gitlab::Git::Checksum).not_to receive(:new).with(storage, relative_path)
+      expect(registry.project.repository).not_to receive(:checksum)
 
       service.execute
     end
@@ -35,7 +31,7 @@ describe Geo::RepositoryVerifySecondaryService, :geo do
     it 'does not verify the checksum if primary was never verified' do
       repository_state.assign_attributes("#{type}_verification_checksum" => nil)
 
-      expect(Gitlab::Git::Checksum).not_to receive(:new).with(storage, relative_path)
+      expect(registry.project.repository).not_to receive(:checksum)
 
       service.execute
     end
@@ -44,22 +40,20 @@ describe Geo::RepositoryVerifySecondaryService, :geo do
       repository_state.assign_attributes("#{type}_verification_checksum" => 'my_checksum')
       registry.assign_attributes("#{type}_verification_checksum_sha" => 'my_checksum')
 
-      expect(Gitlab::Git::Checksum).not_to receive(:new).with(storage, relative_path)
+      expect(registry.project.repository).not_to receive(:checksum)
 
       service.execute
     end
 
     it 'sets checksum when the checksum matches' do
-      expect(Gitlab::Git::Checksum).to receive(:new).with(storage, relative_path) { checksum }
-      expect(checksum).to receive(:calculate).and_return('my_checksum')
+      expect(registry.project.repository).to receive(:checksum).and_return('my_checksum')
 
       expect { service.execute }.to change(registry, "#{type}_verification_checksum_sha")
         .from(nil).to('my_checksum')
     end
 
     it 'keeps track of failure when the checksum mismatch' do
-      expect(Gitlab::Git::Checksum).to receive(:new).with(storage, relative_path) { checksum }
-      expect(checksum).to receive(:calculate).and_return('other_checksum')
+      expect(registry.project.repository).to receive(:checksum).and_return('other_checksum')
 
       expect { service.execute }.to change(registry, "last_#{type}_verification_failure")
         .from(nil).to(/#{Regexp.quote(type.to_s.capitalize)} checksum mismatch/)

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

@@ -20,7 +20,7 @@ describe Geo::RepositoryVerification::Primary::SingleWorker, :postgresql, :clean
     it 'does not calculate the checksum when not running on a primary' do
       allow(Gitlab::Geo).to receive(:primary?) { false }
 
-      expect_any_instance_of(Gitlab::Git::Checksum).not_to receive(:calculate)
+      expect(project_without_repositories.repository).not_to receive(:checksum)
 
       subject.perform(project_without_repositories.id)
     end
@@ -28,7 +28,7 @@ describe Geo::RepositoryVerification::Primary::SingleWorker, :postgresql, :clean
     it 'does not calculate the checksum when project is pending deletion' do
       project_with_repositories.update!(pending_delete: true)
 
-      expect_any_instance_of(Gitlab::Git::Checksum).not_to receive(:calculate)
+      expect(project_with_repositories.repository).not_to receive(:checksum)
 
       subject.perform(project_with_repositories.id)
     end
@@ -126,9 +126,9 @@ describe Geo::RepositoryVerification::Primary::SingleWorker, :postgresql, :clean
 
       expect(project_without_repositories.repository_state).to have_attributes(
         repository_verification_checksum: nil,
-        last_repository_verification_failure: /No repository for such path/,
+        last_repository_verification_failure: /not a git repository/,
         wiki_verification_checksum: nil,
-        last_wiki_verification_failure: /No repository for such path/
+        last_wiki_verification_failure: /not a git repository/
       )
     end
   end

lib/gitlab/git/checksum.rb

-module Gitlab
-  module Git
-    class Checksum
-      include Gitlab::Git::Popen
-
-      EMPTY_REPOSITORY_CHECKSUM = '0000000000000000000000000000000000000000'.freeze
-
-      Failure = Class.new(StandardError)
-
-      attr_reader :path, :relative_path, :storage, :storage_path
-
-      def initialize(storage, relative_path)
-        @storage       = storage
-        @storage_path  = Gitlab.config.repositories.storages[storage].legacy_disk_path
-        @relative_path = "#{relative_path}.git"
-        @path          = File.join(storage_path, @relative_path)
-      end
-
-      def calculate
-        unless repository_exists?
-          failure!(Gitlab::Git::Repository::NoRepository, 'No repository for such path')
-        end
-
-        calculate_checksum_by_shelling_out
-      end
-
-      private
-
-      def repository_exists?
-        raw_repository.exists?
-      end
-
-      def calculate_checksum_by_shelling_out
-        args = %W(--git-dir=#{path} show-ref --heads --tags)
-        output, status = run_git(args)
-
-        if status&.zero?
-          refs = output.split("\n")
-
-          result = refs.inject(nil) do |checksum, ref|
-            value = Digest::SHA1.hexdigest(ref).hex
-
-            if checksum.nil?
-              value
-            else
-              checksum ^ value
-            end
-          end
-
-          result.to_s(16)
-        else
-          # Empty repositories return with a non-zero status and an empty output.
-          if output&.empty?
-            EMPTY_REPOSITORY_CHECKSUM
-          else
-            failure!(Gitlab::Git::Checksum::Failure, output)
-          end
-        end
-      end
-
-      def failure!(klass, message)
-        Gitlab::GitLogger.error("'git show-ref --heads --tags' in #{path}: #{message}")
-
-        raise klass.new("Could not calculate the checksum for #{path}: #{message}")
-      end
-
-      def circuit_breaker
-        @circuit_breaker ||= Gitlab::Git::Storage::CircuitBreaker.for_storage(storage)
-      end
-
-      def raw_repository
-        Gitlab::Git::Repository.new(storage, relative_path, nil)
-      end
-
-      def run_git(args)
-        circuit_breaker.perform do
-          popen([Gitlab.config.git.bin_path, *args], path)
-        end
-      end
-    end
-  end
-end

lib/gitlab/git/repository.rb

@@ -23,6 +23,7 @@ module Gitlab
       SQUASH_WORKTREE_PREFIX = 'squash'.freeze
       GITALY_INTERNAL_URL = 'ssh://gitaly/internal.git'.freeze
       GITLAB_PROJECTS_TIMEOUT = Gitlab.config.gitlab_shell.git_timeout
+      EMPTY_REPOSITORY_CHECKSUM = '0000000000000000000000000000000000000000'.freeze
 
       NoRepository = Class.new(StandardError)
       InvalidBlobName = Class.new(StandardError)
@@ -31,6 +32,7 @@ module Gitlab
       DeleteBranchError = Class.new(StandardError)
       CreateTreeError = Class.new(StandardError)
       TagExistsError = Class.new(StandardError)
+      ChecksumError = Class.new(StandardError)
 
       class << self
         # Unlike `new`, `create` takes the repository path
@@ -1470,6 +1472,16 @@ module Gitlab
         run_git!(['rev-list', '--max-count=1', oldrev, "^#{newrev}"])
       end
 
+      def checksum
+        gitaly_migrate(:calculate_checksum) do |is_enabled|
+          if is_enabled
+            gitaly_repository_client.calculate_checksum
+          else
+            calculate_checksum_by_shelling_out
+          end
+        end
+      end
+
       private
 
       def local_write_ref(ref_path, ref, old_ref: nil, shell: true)
@@ -2423,6 +2435,34 @@ module Gitlab
       def sha_from_ref(ref)
         rev_parse_target(ref).oid
       end
+
+      def calculate_checksum_by_shelling_out
+        raise NoRepository unless exists?
+
+        args = %W(--git-dir=#{path} show-ref --heads --tags)
+        output, status = run_git(args)
+
+        if status.nil? || !status.zero?
+          # Empty repositories return with a non-zero status and an empty output.
+          return EMPTY_REPOSITORY_CHECKSUM if output&.empty?
+
+          raise ChecksumError, output
+        end
+
+        refs = output.split("\n")
+
+        result = refs.inject(nil) do |checksum, ref|
+          value = Digest::SHA1.hexdigest(ref).hex
+
+          if checksum.nil?
+            value
+          else
+            checksum ^ value
+          end
+        end
+
+        result.to_s(16)
+      end
     end
   end
 end

lib/gitlab/gitaly_client/repository_service.rb

@@ -257,6 +257,12 @@ module Gitlab
 
         response.license_short_name.presence
       end
+
+      def calculate_checksum
+        request  = Gitaly::CalculateChecksumRequest.new(repository: @gitaly_repo)
+        response = GitalyClient.call(@storage, :repository_service, :calculate_checksum, request)
+        response.checksum.presence
+      end
     end
   end
 end

spec/lib/gitlab/git/checksum_spec.rb

-require 'spec_helper'
-
-describe Gitlab::Git::Checksum, seed_helper: true do
-  let(:storage) { 'default' }
-
-  it 'raises Gitlab::Git::Repository::NoRepository when there is no repo' do
-    checksum = described_class.new(storage, 'nonexistent-repo')
-
-    expect { checksum.calculate }.to raise_error Gitlab::Git::Repository::NoRepository
-  end
-
-  it 'pretends that checksum is 000000... when the repo is empty' do
-    FileUtils.rm_rf(File.join(SEED_STORAGE_PATH, 'empty-repo.git'))
-
-    system(git_env, *%W(#{Gitlab.config.git.bin_path} init --bare empty-repo.git),
-           chdir: SEED_STORAGE_PATH,
-           out:   '/dev/null',
-           err:   '/dev/null')
-
-    checksum = described_class.new(storage, 'empty-repo')
-
-    expect(checksum.calculate).to eq '0000000000000000000000000000000000000000'
-  end
-
-  it 'raises Gitlab::Git::Repository::Failure when shelling out to git return non-zero status' do
-    checksum = described_class.new(storage, 'gitlab-git-test')
-
-    allow(checksum).to receive(:popen).and_return(['output', nil])
-
-    expect { checksum.calculate }.to raise_error Gitlab::Git::Checksum::Failure
-  end
-
-  it 'calculates the checksum when there is a repo' do
-    checksum = described_class.new(storage, 'gitlab-git-test')
-
-    expect(checksum.calculate).to eq '54f21be4c32c02f6788d72207fa03ad3bce725e4'
-  end
-end

spec/lib/gitlab/git/repository_spec.rb

@@ -2178,6 +2178,55 @@ describe Gitlab::Git::Repository, seed_helper: true do
     end
   end
 
+  describe '#checksum' do
+    shared_examples 'calculating checksum' do
+      it 'calculates the checksum for non-empty repo' do
+        expect(repository.checksum).to eq '54f21be4c32c02f6788d72207fa03ad3bce725e4'
+      end
+
+      it 'returns 0000000000000000000000000000000000000000 for an empty repo' do
+        FileUtils.rm_rf(File.join(storage_path, 'empty-repo.git'))
+
+        system(git_env, *%W(#{Gitlab.config.git.bin_path} init --bare empty-repo.git),
+               chdir: storage_path,
+               out:   '/dev/null',
+               err:   '/dev/null')
+
+        empty_repo = described_class.new('default', 'empty-repo.git', '')
+
+        expect(empty_repo.checksum).to eq '0000000000000000000000000000000000000000'
+      end
+
+      it 'raises a no repository exception when there is no repo' do
+        broken_repo = described_class.new('default', 'a/path.git', '')
+
+        expect { broken_repo.checksum }.to raise_error(Gitlab::Git::Repository::NoRepository)
+      end
+    end
+
+    context 'when calculate_checksum Gitaly feature is enabled' do
+      it_behaves_like 'calculating checksum'
+    end
+
+    context 'when calculate_checksum Gitaly feature is disabled', :disable_gitaly do
+      it_behaves_like 'calculating checksum'
+
+      describe 'when storage is broken', :broken_storage  do
+        it 'raises a storage exception when storage is not available' do
+          broken_repo = described_class.new('broken', 'a/path.git', '')
+
+          expect { broken_repo.rugged }.to raise_error(Gitlab::Git::Storage::Inaccessible)
+        end
+      end
+
+      it "raises a Gitlab::Git::Repository::Failure error if the `popen` call to git returns a non-zero exit code" do
+        allow(repository).to receive(:popen).and_return(['output', nil])
+
+        expect { repository.checksum }.to raise_error Gitlab::Git::Repository::ChecksumError
+      end
+    end
+  end
+
   context 'gitlab_projects commands' do
     let(:gitlab_projects) { repository.gitlab_projects }
     let(:timeout) { Gitlab.config.gitlab_shell.git_timeout }

spec/lib/gitlab/gitaly_client/repository_service_spec.rb

@@ -124,4 +124,15 @@ describe Gitlab::GitalyClient::RepositoryService do
       client.squash_in_progress?(squash_id)
     end
   end
+
+  describe '#calculate_checksum' do
+    it 'sends a calculate_checksum message' do
+      expect_any_instance_of(Gitaly::RepositoryService::Stub)
+        .to receive(:calculate_checksum)
+        .with(gitaly_request_with_path(storage_name, relative_path), kind_of(Hash))
+        .and_return(double(checksum: 0))
+
+      client.calculate_checksum
+    end
+  end
 end