Move GC RPCs to mandatory

Closes https://gitlab.com/gitlab-org/gitaly/issues/354

2 files changed, 52 insertions, 123 deletions

diff --git a/app/workers/git_garbage_collect_worker.rb b/app/workers/git_garbage_collect_worker.rb
index f3c9e2b1582..ae5c5fac834 100644
--- a/app/workers/git_garbage_collect_worker.rb
+++ b/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
diff --git a/spec/workers/git_garbage_collect_worker_spec.rb b/spec/workers/git_garbage_collect_worker_spec.rb
index 807d1b8c084..e39dec556fc 100644
--- a/spec/workers/git_garbage_collect_worker_spec.rb
+++ b/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