Merge branch 'zj-mandatory-batch' into 'master'

Move Gitaly RPCs to mandatory

Closes gitaly#217, gitaly#389, gitaly#390, gitaly#220, gitaly#376, and gitaly#354

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

app/workers/git_garbage_collect_worker.rb

@@ -6,12 +6,6 @@ class GitGarbageCollectWorker
   # Timeout set to 24h
   LEASE_TIMEOUT = 86400
 
-  GITALY_MIGRATED_TASKS = {
-    gc: :garbage_collect,
-    full_repack: :repack_full,
-    incremental_repack: :repack_incremental
-  }.freeze
-
   def perform(project_id, task = :gc, lease_key = nil, lease_uuid = nil)
     project = Project.find(project_id)
     active_uuid = get_lease_uuid(lease_key)
@@ -27,21 +21,7 @@ class GitGarbageCollectWorker
     end
 
     task = task.to_sym
-    cmd = command(task)
-
-    gitaly_migrate(GITALY_MIGRATED_TASKS[task], status: Gitlab::GitalyClient::MigrationStatus::OPT_OUT) do |is_enabled|
-      if is_enabled
-        gitaly_call(task, project.repository.raw_repository)
-      else
-        repo_path = project.repository.path_to_repo
-        description = "'#{cmd.join(' ')}' in #{repo_path}"
-        Gitlab::GitLogger.info(description)
-
-        output, status = Gitlab::Popen.popen(cmd, repo_path)
-
-        Gitlab::GitLogger.error("#{description} failed:\n#{output}") unless status.zero?
-      end
-    end
+    gitaly_call(task, project.repository.raw_repository)
 
     # Refresh the branch cache in case garbage collection caused a ref lookup to fail
     flush_ref_caches(project) if task == :gc
@@ -82,21 +62,12 @@ class GitGarbageCollectWorker
     when :incremental_repack
       client.repack_incremental
     end
-  end
-
-  def command(task)
-    case task
-    when :gc
-      git(write_bitmaps: bitmaps_enabled?) + %w[gc]
-    when :full_repack
-      git(write_bitmaps: bitmaps_enabled?) + %w[repack -A -d --pack-kept-objects]
-    when :incremental_repack
-      # Normal git repack fails when bitmaps are enabled. It is impossible to
-      # create a bitmap here anyway.
-      git(write_bitmaps: false) + %w[repack -d]
-    else
-      raise "Invalid gc task: #{task.inspect}"
-    end
+  rescue GRPC::NotFound => e
+    Gitlab::GitLogger.error("#{method} failed:\nRepository not found")
+    raise Gitlab::Git::Repository::NoRepository.new(e)
+  rescue GRPC::BadStatus => e
+    Gitlab::GitLogger.error("#{method} failed:\n#{e}")
+    raise Gitlab::Git::CommandError.new(e)
   end
 
   def flush_ref_caches(project)
@@ -108,19 +79,4 @@ class GitGarbageCollectWorker
   def bitmaps_enabled?
     Gitlab::CurrentSettings.housekeeping_bitmaps_enabled
   end
-
-  def git(write_bitmaps:)
-    config_value = write_bitmaps ? 'true' : 'false'
-    %W[git -c repack.writeBitmaps=#{config_value}]
-  end
-
-  def gitaly_migrate(method, status: Gitlab::GitalyClient::MigrationStatus::OPT_IN, &block)
-    Gitlab::GitalyClient.migrate(method, status: status, &block)
-  rescue GRPC::NotFound => e
-    Gitlab::GitLogger.error("#{method} failed:\nRepository not found")
-    raise Gitlab::Git::Repository::NoRepository.new(e)
-  rescue GRPC::BadStatus => e
-    Gitlab::GitLogger.error("#{method} failed:\n#{e}")
-    raise Gitlab::Git::CommandError.new(e)
-  end
 end

lib/gitlab/git/blame.rb

@@ -22,24 +22,9 @@ module Gitlab
       private
 
       def load_blame
-        raw_output = @repo.gitaly_migrate(:blame, status: Gitlab::GitalyClient::MigrationStatus::OPT_OUT) do |is_enabled|
-          if is_enabled
-            load_blame_by_gitaly
-          else
-            load_blame_by_shelling_out
-          end
-        end
-
-        output = encode_utf8(raw_output)
-        process_raw_blame output
-      end
-
-      def load_blame_by_gitaly
-        @repo.gitaly_commit_client.raw_blame(@sha, @path)
-      end
+        output = encode_utf8(@repo.gitaly_commit_client.raw_blame(@sha, @path))
 
-      def load_blame_by_shelling_out
-        @repo.shell_blame(@sha, @path)
+        process_raw_blame(output)
       end
 
       def process_raw_blame(output)

lib/gitlab/git/repository.rb

@@ -120,13 +120,11 @@ module Gitlab
 
       # Default branch in the repository
       def root_ref
-        @root_ref ||= gitaly_migrate(:root_ref, status: Gitlab::GitalyClient::MigrationStatus::OPT_OUT) do |is_enabled|
-          if is_enabled
-            gitaly_ref_client.default_branch_name
-          else
-            discover_default_branch
-          end
-        end
+        gitaly_ref_client.default_branch_name
+      rescue GRPC::NotFound => e
+        raise NoRepository.new(e.message)
+      rescue GRPC::Unknown => e
+        raise Gitlab::Git::CommandError.new(e.message)
       end
 
       def rugged
@@ -152,23 +150,15 @@ module Gitlab
       # Returns an Array of branch names
       # sorted by name ASC
       def branch_names
-        gitaly_migrate(:branch_names, status: Gitlab::GitalyClient::MigrationStatus::OPT_OUT) do |is_enabled|
-          if is_enabled
-            gitaly_ref_client.branch_names
-          else
-            branches.map(&:name)
-          end
+        wrapped_gitaly_errors do
+          gitaly_ref_client.branch_names
         end
       end
 
       # Returns an Array of Branches
       def branches
-        gitaly_migrate(:branches, status: Gitlab::GitalyClient::MigrationStatus::OPT_OUT) do |is_enabled|
-          if is_enabled
-            gitaly_ref_client.branches
-          else
-            branches_filter
-          end
+        wrapped_gitaly_errors do
+          gitaly_ref_client.branches
         end
       end
 
@@ -200,12 +190,8 @@ module Gitlab
       end
 
       def local_branches(sort_by: nil)
-        gitaly_migrate(:local_branches, status: Gitlab::GitalyClient::MigrationStatus::OPT_OUT) do |is_enabled|
-          if is_enabled
-            gitaly_ref_client.local_branches(sort_by: sort_by)
-          else
-            branches_filter(filter: :local, sort_by: sort_by)
-          end
+        wrapped_gitaly_errors do
+          gitaly_ref_client.local_branches(sort_by: sort_by)
         end
       end
 
@@ -245,18 +231,6 @@ module Gitlab
       # This refs by default not visible in project page and not cloned to client side.
       alias_method :has_visible_content?, :has_local_branches?
 
-      def has_local_branches_rugged?
-        rugged.branches.each(:local).any? do |ref|
-          begin
-            ref.name && ref.target # ensures the branch is valid
-
-            true
-          rescue Rugged::ReferenceError
-            false
-          end
-        end
-      end
-
       # Returns the number of valid tags
       def tag_count
         gitaly_migrate(:tag_names) do |is_enabled|
@@ -270,12 +244,8 @@ module Gitlab
 
       # Returns an Array of tag names
       def tag_names
-        gitaly_migrate(:tag_names, status: Gitlab::GitalyClient::MigrationStatus::OPT_OUT) do |is_enabled|
-          if is_enabled
-            gitaly_ref_client.tag_names
-          else
-            rugged.tags.map { |t| t.name }
-          end
+        wrapped_gitaly_errors do
+          gitaly_ref_client.tag_names
         end
       end
 
@@ -283,12 +253,8 @@ module Gitlab
       #
       # Gitaly migration: https://gitlab.com/gitlab-org/gitaly/issues/390
       def tags
-        gitaly_migrate(:tags, status: Gitlab::GitalyClient::MigrationStatus::OPT_OUT) do |is_enabled|
-          if is_enabled
-            tags_from_gitaly
-          else
-            tags_from_rugged
-          end
+        wrapped_gitaly_errors do
+          gitaly_ref_client.tags
         end
       end
 
@@ -364,31 +330,6 @@ module Gitlab
         end.map(&:name)
       end
 
-      # Discovers the default branch based on the repository's available branches
-      #
-      # - If no branches are present, returns nil
-      # - If one branch is present, returns its name
-      # - If two or more branches are present, returns current HEAD or master or first branch
-      def discover_default_branch
-        names = branch_names
-
-        return if names.empty?
-
-        return names[0] if names.length == 1
-
-        if rugged_head
-          extracted_name = Ref.extract_branch_name(rugged_head.name)
-
-          return extracted_name if names.include?(extracted_name)
-        end
-
-        if names.include?('master')
-          'master'
-        else
-          names[0]
-        end
-      end
-
       def rugged_head
         rugged.head
       rescue Rugged::ReferenceError
@@ -1453,6 +1394,16 @@ module Gitlab
         raise CommandError.new(e)
       end
 
+      def wrapped_gitaly_errors(&block)
+        yield block
+      rescue GRPC::NotFound => e
+        raise NoRepository.new(e)
+      rescue GRPC::InvalidArgument => e
+        raise ArgumentError.new(e)
+      rescue GRPC::BadStatus => e
+        raise CommandError.new(e)
+      end
+
       def clean_stale_repository_files
         gitaly_migrate(:repository_cleanup, status: Gitlab::GitalyClient::MigrationStatus::OPT_OUT) do |is_enabled|
           gitaly_repository_client.cleanup if is_enabled && exists?
@@ -1606,12 +1557,8 @@ module Gitlab
       private
 
       def uncached_has_local_branches?
-        gitaly_migrate(:has_local_branches, status: Gitlab::GitalyClient::MigrationStatus::OPT_OUT) do |is_enabled|
-          if is_enabled
-            gitaly_repository_client.has_local_branches?
-          else
-            has_local_branches_rugged?
-          end
+        wrapped_gitaly_errors do
+          gitaly_repository_client.has_local_branches?
         end
       end
 
@@ -1731,20 +1678,6 @@ module Gitlab
         }
       end
 
-      # Gitaly note: JV: Trying to get rid of the 'filter' option so we can implement this with 'git'.
-      def branches_filter(filter: nil, sort_by: nil)
-        branches = rugged.branches.each(filter).map do |rugged_ref|
-          begin
-            target_commit = Gitlab::Git::Commit.find(self, rugged_ref.target)
-            Gitlab::Git::Branch.new(self, rugged_ref.name, rugged_ref.target, target_commit)
-          rescue Rugged::ReferenceError
-            # Omit invalid branch
-          end
-        end.compact
-
-        sort_branches(branches, sort_by)
-      end
-
       def git_merged_branch_names(branch_names, root_sha)
         git_arguments =
           %W[branch --merged #{root_sha}
@@ -1956,37 +1889,11 @@ module Gitlab
         end
       end
 
-      def tags_from_rugged
-        rugged.references.each("refs/tags/*").map do |ref|
-          message = nil
-
-          if ref.target.is_a?(Rugged::Tag::Annotation)
-            tag_message = ref.target.message
-
-            if tag_message.respond_to?(:chomp)
-              message = tag_message.chomp
-            end
-          end
-
-          target_commit = Gitlab::Git::Commit.find(self, ref.target)
-          Gitlab::Git::Tag.new(self, {
-            name: ref.name,
-            target: ref.target,
-            target_commit: target_commit,
-            message: message
-          })
-        end.sort_by(&:name)
-      end
-
       def last_commit_for_path_by_rugged(sha, path)
         sha = last_commit_id_for_path_by_shelling_out(sha, path)
         commit(sha)
       end
 
-      def tags_from_gitaly
-        gitaly_ref_client.tags
-      end
-
       def size_by_shelling_out
         popen(%w(du -sk), path).first.strip.to_i
       end

spec/lib/gitlab/git/blame_spec.rb

@@ -7,7 +7,7 @@ describe Gitlab::Git::Blame, seed_helper: true do
     Gitlab::Git::Blame.new(repository, SeedRepo::Commit::ID, "CONTRIBUTING.md")
   end
 
-  shared_examples 'blaming a file' do
+  describe 'blaming a file' do
     context "each count" do
       it do
         data = []
@@ -68,12 +68,4 @@ describe Gitlab::Git::Blame, seed_helper: true do
       end
     end
   end
-
-  context 'when Gitaly blame feature is enabled' do
-    it_behaves_like 'blaming a file'
-  end
-
-  context 'when Gitaly blame feature is disabled', :skip_gitaly_mock do
-    it_behaves_like 'blaming a file'
-  end
 end

spec/lib/gitlab/git/repository_spec.rb

@@ -77,17 +77,6 @@ describe Gitlab::Git::Repository, seed_helper: true do
   end
 
   describe '#root_ref' do
-    context 'with gitaly disabled' do
-      before do
-        allow(Gitlab::GitalyClient).to receive(:feature_enabled?).and_return(false)
-      end
-
-      it 'calls #discover_default_branch' do
-        expect(repository).to receive(:discover_default_branch)
-        repository.root_ref
-      end
-    end
-
     it 'returns UTF-8' do
       expect(repository.root_ref).to be_utf8
     end
@@ -153,46 +142,6 @@ describe Gitlab::Git::Repository, seed_helper: true do
     end
   end
 
-  describe "#discover_default_branch" do
-    let(:master) { 'master' }
-    let(:feature) { 'feature' }
-    let(:feature2) { 'feature2' }
-
-    around do |example|
-      # discover_default_branch will be moved to gitaly-ruby
-      Gitlab::GitalyClient::StorageSettings.allow_disk_access do
-        example.run
-      end
-    end
-
-    it "returns 'master' when master exists" do
-      expect(repository).to receive(:branch_names).at_least(:once).and_return([feature, master])
-      expect(repository.discover_default_branch).to eq('master')
-    end
-
-    it "returns non-master when master exists but default branch is set to something else" do
-      File.write(File.join(repository_path, 'HEAD'), 'ref: refs/heads/feature')
-      expect(repository).to receive(:branch_names).at_least(:once).and_return([feature, master])
-      expect(repository.discover_default_branch).to eq('feature')
-      File.write(File.join(repository_path, 'HEAD'), 'ref: refs/heads/master')
-    end
-
-    it "returns a non-master branch when only one exists" do
-      expect(repository).to receive(:branch_names).at_least(:once).and_return([feature])
-      expect(repository.discover_default_branch).to eq('feature')
-    end
-
-    it "returns a non-master branch when more than one exists and master does not" do
-      expect(repository).to receive(:branch_names).at_least(:once).and_return([feature, feature2])
-      expect(repository.discover_default_branch).to eq('feature')
-    end
-
-    it "returns nil when no branch exists" do
-      expect(repository).to receive(:branch_names).at_least(:once).and_return([])
-      expect(repository.discover_default_branch).to be_nil
-    end
-  end
-
   describe '#branch_names' do
     subject { repository.branch_names }
 
@@ -476,7 +425,7 @@ describe Gitlab::Git::Repository, seed_helper: true do
   end
 
   describe '#has_local_branches?' do
-    shared_examples 'check for local branches' do
+    context 'check for local branches' do
       it { expect(repository.has_local_branches?).to eq(true) }
 
       context 'mutable' do
@@ -510,14 +459,6 @@ describe Gitlab::Git::Repository, seed_helper: true do
         end
       end
     end
-
-    context 'with gitaly' do
-      it_behaves_like 'check for local branches'
-    end
-
-    context 'without gitaly', :skip_gitaly_mock do
-      it_behaves_like 'check for local branches'
-    end
   end
 
   describe "#delete_branch" do
@@ -1395,24 +1336,6 @@ describe Gitlab::Git::Repository, seed_helper: true do
       end
     end
 
-    # With Gitaly enabled, Gitaly just doesn't return deleted branches.
-    context 'with deleted branch with Gitaly disabled' do
-      before do
-        allow(Gitlab::GitalyClient).to receive(:feature_enabled?).and_return(false)
-      end
-
-      it 'returns no results' do
-        ref = double()
-        allow(ref).to receive(:name) { 'bad-branch' }
-        allow(ref).to receive(:target) { raise Rugged::ReferenceError }
-        branches = double()
-        allow(branches).to receive(:each) { [ref].each }
-        allow(repository_rugged).to receive(:branches) { branches }
-
-        expect(subject).to be_empty
-      end
-    end
-
     it_behaves_like 'wrapping gRPC errors', Gitlab::GitalyClient::RefService, :branches
   end
 

spec/workers/git_garbage_collect_worker_spec.rb

@@ -11,36 +11,63 @@ describe GitGarbageCollectWorker do
   subject { described_class.new }
 
   describe "#perform" do
-    shared_examples 'flushing ref caches' do |gitaly|
-      context 'with active lease_uuid' do
+    context 'with active lease_uuid' do
+      before do
+        allow(subject).to receive(:get_lease_uuid).and_return(lease_uuid)
+      end
+
+      it "flushes ref caches when the task if 'gc'" do
+        expect(subject).to receive(:renew_lease).with(lease_key, lease_uuid).and_call_original
+        expect_any_instance_of(Gitlab::GitalyClient::RepositoryService).to receive(:garbage_collect)
+          .and_return(nil)
+        expect_any_instance_of(Repository).to receive(:after_create_branch).and_call_original
+        expect_any_instance_of(Repository).to receive(:branch_names).and_call_original
+        expect_any_instance_of(Repository).to receive(:has_visible_content?).and_call_original
+        expect_any_instance_of(Gitlab::Git::Repository).to receive(:has_visible_content?).and_call_original
+
+        subject.perform(project.id, :gc, lease_key, lease_uuid)
+      end
+    end
+
+    context 'with different lease than the active one' do
+      before do
+        allow(subject).to receive(:get_lease_uuid).and_return(SecureRandom.uuid)
+      end
+
+      it 'returns silently' do
+        expect_any_instance_of(Repository).not_to receive(:after_create_branch).and_call_original
+        expect_any_instance_of(Repository).not_to receive(:branch_names).and_call_original
+        expect_any_instance_of(Repository).not_to receive(:has_visible_content?).and_call_original
+
+        subject.perform(project.id, :gc, lease_key, lease_uuid)
+      end
+    end
+
+    context 'with no active lease' do
+      before do
+        allow(subject).to receive(:get_lease_uuid).and_return(false)
+      end
+
+      context 'when is able to get the lease' do
         before do
-          allow(subject).to receive(:get_lease_uuid).and_return(lease_uuid)
+          allow(subject).to receive(:try_obtain_lease).and_return(SecureRandom.uuid)
         end
 
         it "flushes ref caches when the task if 'gc'" do
-          expect(subject).to receive(:renew_lease).with(lease_key, lease_uuid).and_call_original
-          expect(subject).to receive(:command).with(:gc).and_return([:the, :command])
-
-          if gitaly
-            expect_any_instance_of(Gitlab::GitalyClient::RepositoryService).to receive(:garbage_collect)
-              .and_return(nil)
-          else
-            expect(Gitlab::Popen).to receive(:popen)
-              .with([:the, :command], project.repository.path_to_repo).and_return(["", 0])
-          end
-
+          expect_any_instance_of(Gitlab::GitalyClient::RepositoryService).to receive(:garbage_collect)
+            .and_return(nil)
           expect_any_instance_of(Repository).to receive(:after_create_branch).and_call_original
           expect_any_instance_of(Repository).to receive(:branch_names).and_call_original
           expect_any_instance_of(Repository).to receive(:has_visible_content?).and_call_original
           expect_any_instance_of(Gitlab::Git::Repository).to receive(:has_visible_content?).and_call_original
 
-          subject.perform(project.id, :gc, lease_key, lease_uuid)
+          subject.perform(project.id)
         end
       end
 
-      context 'with different lease than the active one' do
+      context 'when no lease can be obtained' do
         before do
-          allow(subject).to receive(:get_lease_uuid).and_return(SecureRandom.uuid)
+          expect(subject).to receive(:try_obtain_lease).and_return(false)
         end
 
         it 'returns silently' do
@@ -49,63 +76,9 @@ describe GitGarbageCollectWorker do
           expect_any_instance_of(Repository).not_to receive(:branch_names).and_call_original
           expect_any_instance_of(Repository).not_to receive(:has_visible_content?).and_call_original
 
-          subject.perform(project.id, :gc, lease_key, lease_uuid)
+          subject.perform(project.id)
         end
       end
-
-      context 'with no active lease' do
-        before do
-          allow(subject).to receive(:get_lease_uuid).and_return(false)
-        end
-
-        context 'when is able to get the lease' do
-          before do
-            allow(subject).to receive(:try_obtain_lease).and_return(SecureRandom.uuid)
-          end
-
-          it "flushes ref caches when the task if 'gc'" do
-            expect(subject).to receive(:command).with(:gc).and_return([:the, :command])
-
-            if gitaly
-              expect_any_instance_of(Gitlab::GitalyClient::RepositoryService).to receive(:garbage_collect)
-                .and_return(nil)
-            else
-              expect(Gitlab::Popen).to receive(:popen)
-                .with([:the, :command], project.repository.path_to_repo).and_return(["", 0])
-            end
-
-            expect_any_instance_of(Repository).to receive(:after_create_branch).and_call_original
-            expect_any_instance_of(Repository).to receive(:branch_names).and_call_original
-            expect_any_instance_of(Repository).to receive(:has_visible_content?).and_call_original
-            expect_any_instance_of(Gitlab::Git::Repository).to receive(:has_visible_content?).and_call_original
-
-            subject.perform(project.id)
-          end
-        end
-
-        context 'when no lease can be obtained' do
-          before do
-            expect(subject).to receive(:try_obtain_lease).and_return(false)
-          end
-
-          it 'returns silently' do
-            expect(subject).not_to receive(:command)
-            expect_any_instance_of(Repository).not_to receive(:after_create_branch).and_call_original
-            expect_any_instance_of(Repository).not_to receive(:branch_names).and_call_original
-            expect_any_instance_of(Repository).not_to receive(:has_visible_content?).and_call_original
-
-            subject.perform(project.id)
-          end
-        end
-      end
-    end
-
-    context "with Gitaly turned on" do
-      it_should_behave_like 'flushing ref caches', true
-    end
-
-    context "with Gitaly turned off", :disable_gitaly do
-      it_should_behave_like 'flushing ref caches', false
     end
 
     context "repack_full" do