Merge branch 'improve-file_registry_removal_service' into 'master'

Refactoring FileRegistryRemoval service  and specs

See merge request gitlab-org/gitlab!77539

ee/app/services/geo/file_registry_removal_service.rb

 # frozen_string_literal: true
 
 module Geo
+  ##
+  ## Geo::FileRegistryRemovalService handles blob removal from a secondary node,
+  ## the file itself and the database records.
+  ## It handles all the possible combinations of 4 variables:
+  ## * Whether Model record exists
+  ## * Whether Registry Record exists
+  ## * Whether the file on a storage exists
+  ## * Whether the file_path is passed (RegistryConsistencyWorker doesn't pass one)
+  ## In all the cases the best effort should have to be made.
+  ##
   class FileRegistryRemovalService < BaseFileService
     include ::Gitlab::Utils::StrongMemoize
 
@@ -73,16 +83,10 @@ module Geo
       end
     end
 
-    def blob_path_from_replicator
-      replicator.blob_path
-    rescue ActiveRecord::RecordNotFound
-      nil
-    end
-
     def file_path
       strong_memoize(:file_path) do
         next @object_file_path if @object_file_path
-        next blob_path_from_replicator if replicator
+
         # 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
@@ -93,6 +97,8 @@ module Geo
 
     def file_uploader
       strong_memoize(:file_uploader) do
+        next replicator.carrierwave_uploader if replicator
+
         case object_type
         when :job_artifact
           Ci::JobArtifact.find(object_db_id).file

ee/spec/services/geo/file_registry_removal_service_spec.rb

@@ -21,6 +21,7 @@ RSpec.describe Geo::FileRegistryRemovalService, :geo do
       described_class.new(:lfs, 99).execute
     end
 
+    context 'with job artifact' do
       shared_examples 'removes artifact' do
         subject(:service) { described_class.new('job_artifact', registry.artifact_id) }
 
@@ -57,42 +58,48 @@ RSpec.describe Geo::FileRegistryRemovalService, :geo do
         end
       end
 
-    shared_examples 'removes LFS object' do
-      subject(:service) { described_class.new('lfs', registry.lfs_object_id) }
+      let!(:job_artifact) { create(:ci_job_artifact, :archive) }
+      let!(:registry) { create(:geo_job_artifact_registry, artifact_id: job_artifact.id) }
+      let!(:file_path) { job_artifact.file.path }
 
+      it_behaves_like 'removes artifact'
+
+      context 'migrated to object storage' do
         before do
-        stub_exclusive_lease("file_registry_removal_service:lfs:#{registry.lfs_object_id}",
-          timeout: Geo::FileRegistryRemovalService::LEASE_TIMEOUT)
+          job_artifact.update_column(:file_store, ObjectStorage::Store::REMOTE)
         end
 
-      it 'file from disk' do
-        expect do
-          service.execute
-        end.to change { File.exist?(file_path) }.from(true).to(false)
+        context 'with object storage enabled' do
+          before do
+            stub_artifacts_object_storage
           end
 
-      it 'deletes registry entry' do
-        expect do
-          service.execute
-        end.to change(Geo::LfsObjectRegistry, :count).by(-1)
+          it_behaves_like 'removes artifact'
         end
+
+        context 'with object storage disabled' do
+          before do
+            stub_artifacts_object_storage(enabled: false)
           end
 
-    shared_examples 'removes LFS object registry' do
-      subject(:service) { described_class.new('lfs', registry.lfs_object_id) }
+          it_behaves_like 'removes artifact registry'
+        end
+      end
 
+      context 'no job artifact record' do
         before do
-        stub_exclusive_lease("file_registry_removal_service:lfs:#{registry.lfs_object_id}",
-          timeout: Geo::FileRegistryRemovalService::LEASE_TIMEOUT)
+          job_artifact.delete
         end
 
-      it 'deletes registry entry' do
-        expect do
-          service.execute
-        end.to change(Geo::LfsObjectRegistry, :count).by(-1)
+        it_behaves_like 'removes artifact' do
+          subject(:service) { described_class.new('job_artifact', registry.artifact_id, file_path) }
+        end
+
+        it_behaves_like 'removes artifact registry'
       end
     end
 
+    context 'with package file' do
       shared_examples 'removes package file' do
         subject(:service) { described_class.new('package_file', registry.package_file_id) }
 
@@ -129,118 +136,108 @@ RSpec.describe Geo::FileRegistryRemovalService, :geo do
         end
       end
 
-    context 'with job artifact' do
-      let!(:job_artifact) { create(:ci_job_artifact, :archive) }
-      let!(:registry) { create(:geo_job_artifact_registry, artifact_id: job_artifact.id) }
-      let!(:file_path) { job_artifact.file.path }
+      let(:package_file) { create(:package_file_with_file) }
+      let!(:registry) { create(:geo_package_file_registry, package_file: package_file) }
+      let(:file_path) { package_file.file.path }
 
-      it_behaves_like 'removes artifact'
+      it_behaves_like 'removes package file'
 
-      context 'migrated to object storage' do
+      context 'no package file record' do
         before do
-          stub_artifacts_object_storage
-          job_artifact.update_column(:file_store, JobArtifactUploader::Store::REMOTE)
-        end
-
-        it_behaves_like 'removes artifact'
+          package_file.delete
         end
 
-      context 'migrated to object storage' do
-        before do
-          stub_artifacts_object_storage
-          job_artifact.update_column(:file_store, LfsObjectUploader::Store::REMOTE)
+        it_behaves_like 'removes package file' do
+          subject(:service) { described_class.new('package_file', registry.package_file_id, file_path) }
         end
-
-        context 'with object storage enabled' do
-          it_behaves_like 'removes artifact'
       end
 
-        context 'with object storage disabled' do
+      context 'with orphaned registry' do
         before do
-            stub_artifacts_object_storage(enabled: false)
+          package_file.delete
         end
 
-          it_behaves_like 'removes artifact registry'
+        it_behaves_like 'removes package file registry' do
+          subject(:service) { described_class.new('package_file', registry.package_file_id) }
         end
       end
-
-      context 'no job artifact record' do
-        before do
-          job_artifact.delete
     end
 
-        it_behaves_like 'removes artifact' do
-          subject(:service) { described_class.new('job_artifact', registry.artifact_id, file_path) }
-        end
-      end
+    context 'with uploads' do
+      shared_examples 'removes upload' do
+        subject(:service) { described_class.new('upload', registry.file_id, file_path) }
 
-      context 'with orphaned registry' do
         before do
-          job_artifact.delete
+          stub_exclusive_lease("file_registry_removal_service:upload:#{registry.file_id}",
+            timeout: Geo::FileRegistryRemovalService::LEASE_TIMEOUT)
         end
 
-        it_behaves_like 'removes artifact registry' do
-          subject(:service) { described_class.new('job_artifact', registry.artifact_id) }
+        it 'file from disk' do
+          expect do
+            service.execute
+          end.to change { File.exist?(file_path) }.from(true).to(false)
         end
+
+        it 'deletes registry entry' do
+          expect do
+            service.execute
+          end.to change(Geo::UploadRegistry, :count).by(-1)
         end
       end
 
-    context 'with package file' do
-      let(:package_file) { create(:package_file_with_file) }
-      let!(:registry) { create(:geo_package_file_registry, package_file: package_file) }
-      let(:file_path) { Tempfile.new.path }
+      shared_examples 'removes upload registry' do
+        subject(:service) { described_class.new('upload', registry.file_id, file_path) }
 
         before do
-        allow_next_instance_of(Geo::PackageFileReplicator) do |replicator|
-          allow(replicator).to receive(:blob_path).and_return(file_path)
+          stub_exclusive_lease("file_registry_removal_service:upload:#{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)
         end
       end
 
-      it_behaves_like 'removes package file'
+      let!(:upload) { create(:upload, :with_file) }
+      let!(:registry) { create(:geo_upload_registry, file_id: upload.id) }
+      let!(:file_path) { upload.retrieve_uploader.file.path }
 
-      context 'no package file record' do
+      it_behaves_like 'removes upload'
+
+      context 'migrated to object storage' do
         before do
-          package_file.delete
+          upload.update_column(:store, ObjectStorage::Store::REMOTE)
         end
 
-        it_behaves_like 'removes package file' do
-          subject(:service) { described_class.new('package_file', registry.package_file_id, file_path) }
+        context 'with object storage enabled' do
+          before do
+            stub_uploads_object_storage(AvatarUploader)
           end
+
+          it_behaves_like 'removes upload'
         end
 
-      context 'with orphaned registry' do
+        context 'with object storage disabled' do
           before do
-          package_file.delete
+            stub_uploads_object_storage(AvatarUploader, enabled: false)
           end
 
-        it_behaves_like 'removes package file registry' do
-          subject(:service) { described_class.new('package_file', registry.package_file_id) }
+          it_behaves_like 'removes upload registry'
         end
       end
-    end
-
-    context 'with uploads' do
-      let!(:upload) { create(:user, :with_avatar).avatar.upload }
-      let!(:registry) { create(:geo_upload_registry, file_id: upload.id) }
-      let!(:file_path) { upload.retrieve_uploader.file.path }
-
-      subject(:service) { described_class.new('upload', registry.file_id) }
 
+      context 'no upload record' do
         before do
-        stub_exclusive_lease("file_registry_removal_service:upload:#{registry.file_id}",
-          timeout: Geo::FileRegistryRemovalService::LEASE_TIMEOUT)
+          upload.delete
         end
 
-      it 'file from disk' do
-        expect do
-          service.execute
-        end.to change { File.exist?(file_path) }.from(true).to(false)
+        it_behaves_like 'removes upload' do
+          subject(:service) { described_class.new('upload', registry.file_id, file_path) }
         end
 
-      it 'deletes registry entry' do
-        expect do
-          service.execute
-        end.to change(Geo::UploadRegistry, :count).by(-1)
+        it_behaves_like 'removes upload registry'
       end
     end
   end