Merge branch...

Merge branch '219993-geo-secondary-tries-to-remove-a-file-indefinitely-when-sync-object-storage-is-enabled' into 'master'

Geo - Does not try to unlink file when unable to get file path while cleaning orphaned registries

Closes #219993

See merge request gitlab-org/gitlab!33658

ee/app/services/geo/file_registry_removal_service.rb

@@ -6,9 +6,9 @@ module Geo
 
     LEASE_TIMEOUT = 8.hours.freeze
 
-    # It's possible that LfsObject or Ci::JobArtifact record does not exist anymore
-    # In this case, you need to pass file_path parameter explicitly
-    #
+    # There is a possibility that the replicable's record does not exist
+    # anymore. In this case, you need to pass the file_path parameter
+    # explicitly.
     def initialize(object_type, object_db_id, file_path = nil)
       @object_type = object_type.to_sym
       @object_db_id = object_db_id
@@ -26,9 +26,11 @@ module Geo
           break
         end
 
-        if File.exist?(file_path)
+        if file_path && File.exist?(file_path)
           log_info('Unlinking file', file_path: file_path)
           File.unlink(file_path)
+        else
+          log_error('Unable to unlink file because file path is unknown. A file may be orphaned', object_type: object_type, object_db_id: object_db_id)
         end
 
         log_info('Removing file registry', file_registry_id: file_registry.id)
@@ -61,6 +63,7 @@ module Geo
       strong_memoize(:file_path) do
         next @object_file_path if @object_file_path
         # When local storage is used, just rely on the existing methods
+        next if file_uploader.nil?
         next file_uploader.file.path if file_uploader.object_store == ObjectStorage::Store::LOCAL
 
         # For remote storage more juggling is needed to actually get the full path on disk
@@ -85,10 +88,19 @@ module Geo
         else
           raise NameError, "Unrecognized type: #{object_type}"
         end
+      rescue RuntimeError, NameError, ActiveRecord::RecordNotFound => err
+        # When cleaning up registries, there are some cases where
+        # it's impossible to unlink the file:
+        #
+        # 1. The replicable record does not exist anymore;
+        # 2. The replicable file is stored on Object Storage,
+        #    but the node is not configured to use Object Store;
+        # 3. Unrecognized replicable type;
+        #
+        log_error('Could not build uploader', err.message)
+
+        nil
       end
-    rescue NameError, ActiveRecord::RecordNotFound => err
-      log_error('Could not build uploader', err.message)
-      raise
     end
 
     def lease_key

ee/changelogs/unreleased/219993-geo-secondary-tries-to-remove-a-file-indefinitely-when-sync-obj.yml

+---
+title: Geo - Does not try to unlink file when it not possible to get the file path
+  while cleaning orphaned registries
+merge_request: 33658
+author:
+type: fixed

ee/spec/services/geo/file_registry_removal_service_spec.rb

@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-describe Geo::FileRegistryRemovalService do
+describe Geo::FileRegistryRemovalService, :geo do
   include ::EE::GeoHelpers
   include ExclusiveLeaseHelpers
 
@@ -35,7 +35,22 @@ describe Geo::FileRegistryRemovalService do
         end.to change { File.exist?(file_path) }.from(true).to(false)
       end
 
-      it 'registry when file was deleted successfully' do
+      it 'deletes registry entry' do
+        expect do
+          service.execute
+        end.to change(Geo::UploadRegistry, :count).by(-1)
+      end
+    end
+
+    shared_examples 'removes registry entry' do
+      subject(:service) { described_class.new(registry.file_type, registry.file_id) }
+
+      before do
+        stub_exclusive_lease("file_registry_removal_service:#{registry.file_type}:#{registry.file_id}",
+          timeout: Geo::FileRegistryRemovalService::LEASE_TIMEOUT)
+      end
+
+      it 'deletes registry entry' do
         expect do
           service.execute
         end.to change(Geo::UploadRegistry, :count).by(-1)
@@ -56,7 +71,22 @@ describe Geo::FileRegistryRemovalService do
         end.to change { File.exist?(file_path) }.from(true).to(false)
       end
 
-      it 'registry when file was deleted successfully' do
+      it 'deletes registry entry' do
+        expect do
+          service.execute
+        end.to change(Geo::JobArtifactRegistry, :count).by(-1)
+      end
+    end
+
+    shared_examples 'removes artifact registry' do
+      subject(:service) { described_class.new('job_artifact', registry.artifact_id) }
+
+      before do
+        stub_exclusive_lease("file_registry_removal_service:job_artifact:#{registry.artifact_id}",
+          timeout: Geo::FileRegistryRemovalService::LEASE_TIMEOUT)
+      end
+
+      it 'deletes registry entry' do
         expect do
           service.execute
         end.to change(Geo::JobArtifactRegistry, :count).by(-1)
@@ -77,7 +107,22 @@ describe Geo::FileRegistryRemovalService do
         end.to change { File.exist?(file_path) }.from(true).to(false)
       end
 
-      it 'registry when file was deleted successfully' do
+      it 'deletes registry entry' do
+        expect do
+          service.execute
+        end.to change(Geo::LfsObjectRegistry, :count).by(-1)
+      end
+    end
+
+    shared_examples 'removes LFS object registry' do
+      subject(:service) { described_class.new('lfs', registry.lfs_object_id) }
+
+      before do
+        stub_exclusive_lease("file_registry_removal_service:lfs:#{registry.lfs_object_id}",
+          timeout: Geo::FileRegistryRemovalService::LEASE_TIMEOUT)
+      end
+
+      it 'deletes registry entry' do
         expect do
           service.execute
         end.to change(Geo::LfsObjectRegistry, :count).by(-1)
@@ -97,7 +142,17 @@ describe Geo::FileRegistryRemovalService do
           lfs_object.update_column(:file_store, LfsObjectUploader::Store::REMOTE)
         end
 
-        it_behaves_like 'removes LFS object'
+        context 'with object storage enabled' do
+          it_behaves_like 'removes LFS object'
+        end
+
+        context 'with object storage disabled' do
+          before do
+            stub_lfs_object_storage(enabled: false)
+          end
+
+          it_behaves_like 'removes LFS object registry'
+        end
       end
 
       context 'no lfs_object record' do
@@ -109,6 +164,16 @@ describe Geo::FileRegistryRemovalService do
           subject(:service) { described_class.new('lfs', registry.lfs_object_id, file_path) }
         end
       end
+
+      context 'with orphaned registry' do
+        before do
+          lfs_object.delete
+        end
+
+        it_behaves_like 'removes LFS object registry' do
+          subject(:service) { described_class.new('lfs', registry.lfs_object_id) }
+        end
+      end
     end
 
     context 'with job artifact' do
@@ -127,6 +192,25 @@ describe Geo::FileRegistryRemovalService do
         it_behaves_like 'removes artifact'
       end
 
+      context 'migrated to object storage' do
+        before do
+          stub_artifacts_object_storage
+          job_artifact.update_column(:file_store, LfsObjectUploader::Store::REMOTE)
+        end
+
+        context 'with object storage enabled' do
+          it_behaves_like 'removes artifact'
+        end
+
+        context 'with object storage disabled' do
+          before do
+            stub_artifacts_object_storage(enabled: false)
+          end
+
+          it_behaves_like 'removes artifact registry'
+        end
+      end
+
       context 'no job artifact record' do
         before do
           job_artifact.delete
@@ -136,6 +220,16 @@ describe Geo::FileRegistryRemovalService do
           subject(:service) { described_class.new('job_artifact', registry.artifact_id, file_path) }
         end
       end
+
+      context 'with orphaned registry' do
+        before do
+          job_artifact.delete
+        end
+
+        it_behaves_like 'removes artifact registry' do
+          subject(:service) { described_class.new('job_artifact', registry.artifact_id) }
+        end
+      end
     end
 
     context 'with avatar' do
@@ -151,7 +245,17 @@ describe Geo::FileRegistryRemovalService do
           upload.update_column(:store, AvatarUploader::Store::REMOTE)
         end
 
-        it_behaves_like 'removes'
+        context 'with object storage enabled' do
+          it_behaves_like 'removes'
+        end
+
+        context 'with object storage disabled' do
+          before do
+            stub_uploads_object_storage(AvatarUploader, enabled: false)
+          end
+
+          it_behaves_like 'removes registry entry'
+        end
       end
     end
 
@@ -168,24 +272,17 @@ describe Geo::FileRegistryRemovalService do
           upload.update_column(:store, AttachmentUploader::Store::REMOTE)
         end
 
-        it_behaves_like 'removes'
-      end
-    end
-
-    context 'with file' do
-      let!(:upload) { create(:user, :with_avatar).avatar.upload }
-      let!(:registry) { create(:geo_upload_registry, :avatar, file_id: upload.id) }
-      let!(:file_path) { upload.retrieve_uploader.file.path }
+        context 'with object storage enabled' do
+          it_behaves_like 'removes'
+        end
 
-      it_behaves_like 'removes'
+        context 'with object storage disabled' do
+          before do
+            stub_uploads_object_storage(AttachmentUploader, enabled: false)
+          end
 
-      context 'migrated to object storage' do
-        before do
-          stub_uploads_object_storage(AvatarUploader)
-          upload.update_column(:store, AvatarUploader::Store::REMOTE)
+          it_behaves_like 'removes registry entry'
         end
-
-        it_behaves_like 'removes'
       end
     end
 
@@ -208,7 +305,17 @@ describe Geo::FileRegistryRemovalService do
           upload.update_column(:store, NamespaceFileUploader::Store::REMOTE)
         end
 
-        it_behaves_like 'removes'
+        context 'with object storage enabled' do
+          it_behaves_like 'removes'
+        end
+
+        context 'with object storage disabled' do
+          before do
+            stub_uploads_object_storage(NamespaceFileUploader, enabled: false)
+          end
+
+          it_behaves_like 'removes registry entry'
+        end
       end
     end
 
@@ -222,15 +329,23 @@ describe Geo::FileRegistryRemovalService do
       let!(:registry) { create(:geo_upload_registry, :personal_file, file_id: upload.id) }
       let!(:file_path) { upload.retrieve_uploader.file.path }
 
-      it_behaves_like 'removes'
-
       context 'migrated to object storage' do
         before do
           stub_uploads_object_storage(PersonalFileUploader)
           upload.update_column(:store, PersonalFileUploader::Store::REMOTE)
         end
 
-        it_behaves_like 'removes'
+        context 'with object storage enabled' do
+          it_behaves_like 'removes'
+        end
+
+        context 'with object storage disabled' do
+          before do
+            stub_uploads_object_storage(PersonalFileUploader, enabled: false)
+          end
+
+          it_behaves_like 'removes registry entry'
+        end
       end
     end
 
@@ -249,10 +364,20 @@ describe Geo::FileRegistryRemovalService do
       context 'migrated to object storage' do
         before do
           stub_uploads_object_storage(FaviconUploader)
-          upload.update_column(:store, PersonalFileUploader::Store::REMOTE)
+          upload.update_column(:store, FaviconUploader::Store::REMOTE)
+        end
+
+        context 'with object storage enabled' do
+          it_behaves_like 'removes'
         end
 
-        it_behaves_like 'removes'
+        context 'with object storage disabled' do
+          before do
+            stub_uploads_object_storage(FaviconUploader, enabled: false)
+          end
+
+          it_behaves_like 'removes registry entry'
+        end
       end
     end
   end