Merge branch '4893-geo-leftover-temporary-directories-from-failed-clones' into 'master'

Resolve "Geo: Leftover temporary directories from failed clones"

Closes #4893 and #5036

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

ee/app/services/geo/base_sync_service.rb

@@ -54,6 +54,7 @@ module Geo
 
     def fetch_repository(redownload)
       log_info("Trying to fetch #{type}")
+      clean_up_temporary_repository
       update_registry!(started_at: DateTime.now)
 
       if redownload
@@ -157,17 +158,13 @@ module Geo
       registry.public_send("last_#{type}_synced_at") # rubocop:disable GitlabSecurity/PublicSend
     end
 
-    def random_disk_path(prefix)
-      random_string = SecureRandom.hex(7)
-      "#{repository.disk_path}_#{prefix}#{random_string}"
-    end
-
     def disk_path_temp
-      @disk_path_temp ||= random_disk_path('')
+      # We use "@" as it's not allowed to use it in a group or project name
+      @disk_path_temp ||= "@geo-temporary/#{repository.disk_path}"
     end
 
     def deleted_disk_path_temp
-      @deleted_path ||= "#{repository.disk_path}+failed-geo-sync"
+      @deleted_path ||= "@failed-geo-sync/#{repository.disk_path}"
     end
 
     def build_temporary_repository
@@ -175,16 +172,17 @@ module Geo
         raise Gitlab::Shell::Error, 'Can not create a temporary repository'
       end
 
-      log_info(
-        'Created temporary repository',
-        temp_path: disk_path_temp
-      )
+      log_info("Created temporary repository")
 
       repository.clone.tap { |repo| repo.disk_path = disk_path_temp }
     end
 
     def clean_up_temporary_repository
-      gitlab_shell.remove_repository(project.repository_storage_path, disk_path_temp)
+      exists = gitlab_shell.exists?(project.repository_storage_path, disk_path_temp)
+
+      if exists && !gitlab_shell.remove_repository(project.repository_storage_path, disk_path_temp)
+        raise Gitlab::Shell::Error, "Temporary #{type} can not been removed"
+      end
     end
 
     def set_temp_repository_as_main
@@ -199,6 +197,9 @@ module Geo
       # Remove the deleted path in case it exists, but it may not be there
       gitlab_shell.remove_repository(project.repository_storage_path, deleted_disk_path_temp)
 
+      # Make sure we have a namespace directory
+      gitlab_shell.add_namespace(project.repository_storage_path, deleted_disk_path_temp)
+
       if project.repository_exists? && !gitlab_shell.mv_repository(project.repository_storage_path, repository.disk_path, deleted_disk_path_temp)
         raise Gitlab::Shell::Error, 'Can not move original repository out of the way'
       end

ee/changelogs/unreleased-ee/4893-geo-leftover-temporary-directories-from-failed-clones.yml

+---
+title: 'Fix Geo: Leftover temporary directories from failed clones'
+merge_request:
+author:
+type: fixed

ee/spec/services/geo/repository_sync_service_spec.rb

@@ -129,6 +129,12 @@ describe Geo::RepositorySyncService do
     end
 
     context 'tracking database' do
+      context 'temporary repositories' do
+        include_examples 'cleans temporary repositories' do
+          let(:repository) { project.repository }
+        end
+      end
+
       it 'creates a new registry if does not exists' do
         expect { subject.execute }.to change(Geo::ProjectRegistry, :count).by(1)
       end
@@ -236,7 +242,7 @@ describe Geo::RepositorySyncService do
 
         expect(subject).to receive(:sync_repository).with(true).and_call_original
         expect(subject.gitlab_shell).to receive(:mv_repository).exactly(2).times.and_call_original
-        expect(subject.gitlab_shell).to receive(:remove_repository).exactly(3).times.and_call_original
+        expect(subject.gitlab_shell).to receive(:remove_repository).exactly(2).times.and_call_original
 
         subject.execute
 
@@ -260,6 +266,20 @@ describe Geo::RepositorySyncService do
         subject.execute
       end
 
+      it 'cleans temporary repo after redownload' do
+        create(
+          :geo_project_registry,
+          project: project,
+          repository_retry_count: Geo::BaseSyncService::RETRY_BEFORE_REDOWNLOAD - 1,
+          force_to_redownload_repository: true
+        )
+
+        expect(subject).to receive(:fetch_geo_mirror)
+        expect(subject).to receive(:clean_up_temporary_repository).twice
+
+        subject.execute
+      end
+
       it 'successfully redownloads the repository even if the retry time exceeds max value' do
         timestamp = Time.now.utc
         registry = create(

ee/spec/services/geo/wiki_sync_service_spec.rb

@@ -108,6 +108,12 @@ RSpec.describe Geo::WikiSyncService do
     end
 
     context 'tracking database' do
+      context 'temporary repositories' do
+        include_examples 'cleans temporary repositories' do
+          let(:repository) { project.wiki.repository }
+        end
+      end
+
       it 'creates a new registry if does not exists' do
         expect { subject.execute }.to change(Geo::ProjectRegistry, :count).by(1)
       end

ee/spec/support/services/base_sync_service_shared_examples.rb

@@ -29,3 +29,21 @@ shared_examples 'geo base sync execution' do
     end
   end
 end
+
+shared_examples 'cleans temporary repositories' do
+  context 'there is a leftover repository' do
+    let(:temp_repo_path) { "@geo-temporary/#{repository.disk_path}" }
+
+    it 'removes leftover repository' do
+      gitlab_shell = instance_double('Gitlab::Shell')
+
+      allow(subject).to receive(:gitlab_shell).and_return(gitlab_shell)
+      allow(subject).to receive(:fetch_geo_mirror)
+
+      expect(gitlab_shell).to receive(:exists?).and_return(true)
+      expect(gitlab_shell).to receive(:remove_repository).with(project.repository_storage_path, temp_repo_path)
+
+      subject.execute
+    end
+  end
+end