Merge branch 'mk/add-orphan-upload-file-task-tests' into 'master'

Add object storage related tests for `gitlab:cleanup:project_uploads` task

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

lib/gitlab/cleanup/project_uploads.rb

@@ -40,7 +40,7 @@ module Gitlab
       # Accepts a path in the form of "#{hex_secret}/#{filename}"
       def find_correct_path(upload_path)
         upload = Upload.find_by(uploader: 'FileUploader', path: upload_path)
-        return unless upload && upload.local?
+        return unless upload && upload.local? && upload.model
 
         upload.absolute_path
       rescue => e

spec/lib/gitlab/cleanup/project_uploads_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::Cleanup::ProjectUploads do
+  subject { described_class.new(logger: logger) }
+  let(:logger) { double(:logger) }
+
+  before do
+    allow(logger).to receive(:info).at_least(1).times
+    allow(logger).to receive(:debug).at_least(1).times
+  end
+
+  describe '#run!' do
+    shared_examples_for 'moves the file' do
+      shared_examples_for 'a real run' do
+        let(:args) { [dry_run: false] }
+
+        it 'moves the file to its proper location' do
+          subject.run!(*args)
+
+          expect(File.exist?(path)).to be_falsey
+          expect(File.exist?(new_path)).to be_truthy
+        end
+
+        it 'logs action as done' do
+          expect(logger).to receive(:info).with("Looking for orphaned project uploads to clean up...")
+          expect(logger).to receive(:info).with("Did #{action}")
+
+          subject.run!(*args)
+        end
+      end
+
+      shared_examples_for 'a dry run' do
+        it 'does not move the file' do
+          subject.run!(*args)
+
+          expect(File.exist?(path)).to be_truthy
+          expect(File.exist?(new_path)).to be_falsey
+        end
+
+        it 'logs action as able to be done' do
+          expect(logger).to receive(:info).with("Looking for orphaned project uploads to clean up. Dry run...")
+          expect(logger).to receive(:info).with("Can #{action}")
+
+          subject.run!(*args)
+        end
+      end
+
+      context 'when dry_run is false' do
+        let(:args) { [dry_run: false] }
+
+        it_behaves_like 'a real run'
+      end
+
+      context 'when dry_run is nil' do
+        let(:args) { [dry_run: nil] }
+
+        it_behaves_like 'a real run'
+      end
+
+      context 'when dry_run is true' do
+        let(:args) { [dry_run: true] }
+
+        it_behaves_like 'a dry run'
+      end
+
+      context 'with dry_run not specified' do
+        let(:args) { [] }
+
+        it_behaves_like 'a dry run'
+      end
+    end
+
+    shared_examples_for 'moves the file to lost and found' do
+      let(:action) { "move to lost and found #{path} -> #{new_path}" }
+
+      it_behaves_like 'moves the file'
+    end
+
+    shared_examples_for 'fixes the file' do
+      let(:action) { "fix #{path} -> #{new_path}" }
+
+      it_behaves_like 'moves the file'
+    end
+
+    context 'orphaned project upload file' do
+      context 'when an upload record matching the secret and filename is found' do
+        context 'when the project is still in legacy storage' do
+          let(:orphaned) { create(:upload, :issuable_upload, :with_file, model: create(:project, :legacy_storage)) }
+          let(:new_path) { orphaned.absolute_path }
+          let(:path) { File.join(FileUploader.root, 'some', 'wrong', 'location', orphaned.path) }
+
+          before do
+            FileUtils.mkdir_p(File.dirname(path))
+            FileUtils.mv(new_path, path)
+          end
+
+          it_behaves_like 'fixes the file'
+        end
+
+        context 'when the project was moved to hashed storage' do
+          let(:orphaned) { create(:upload, :issuable_upload, :with_file) }
+          let(:new_path) { orphaned.absolute_path }
+          let(:path) { File.join(FileUploader.root, 'some', 'wrong', 'location', orphaned.path) }
+
+          before do
+            FileUtils.mkdir_p(File.dirname(path))
+            FileUtils.mv(new_path, path)
+          end
+
+          it_behaves_like 'fixes the file'
+        end
+
+        context 'when the project is missing (the upload *record* is an orphan)' do
+          let(:orphaned) { create(:upload, :issuable_upload, :with_file, model: build(:project, :legacy_storage)) }
+          let!(:path) { orphaned.absolute_path }
+          let!(:new_path) { File.join(FileUploader.root, '-', 'project-lost-found', orphaned.model.full_path, orphaned.path) }
+
+          before do
+            orphaned.model.delete
+          end
+
+          it_behaves_like 'moves the file to lost and found'
+        end
+
+        # We will probably want to add logic (Reschedule background upload) to
+        # cover Case 2 in https://gitlab.com/gitlab-org/gitlab-ce/issues/46535#note_75355104
+        context 'when the file should be in object storage' do
+          context 'when the file otherwise has the correct local path' do
+            let!(:orphaned) { create(:upload, :issuable_upload, :object_storage, model: build(:project, :legacy_storage)) }
+            let!(:path) { File.join(FileUploader.root, orphaned.model.full_path, orphaned.path) }
+
+            before do
+              stub_feature_flags(import_export_object_storage: true)
+              stub_uploads_object_storage(FileUploader)
+
+              FileUtils.mkdir_p(File.dirname(path))
+              FileUtils.touch(path)
+            end
+
+            it 'does not move the file' do
+              expect(File.exist?(path)).to be_truthy
+
+              subject.run!(dry_run: false)
+
+              expect(File.exist?(path)).to be_truthy
+            end
+          end
+
+          # E.g. the upload file was orphaned, and then uploads were migrated to
+          # object storage
+          context 'when the file has the wrong local path' do
+            let!(:orphaned) { create(:upload, :issuable_upload, :object_storage, model: build(:project, :legacy_storage)) }
+            let!(:path) { File.join(FileUploader.root, 'wrong', orphaned.path) }
+            let!(:new_path) { File.join(FileUploader.root, '-', 'project-lost-found', 'wrong', orphaned.path) }
+
+            before do
+              stub_feature_flags(import_export_object_storage: true)
+              stub_uploads_object_storage(FileUploader)
+
+              FileUtils.mkdir_p(File.dirname(path))
+              FileUtils.touch(path)
+            end
+
+            it_behaves_like 'moves the file to lost and found'
+          end
+        end
+      end
+
+      context 'when a matching upload record can not be found' do
+        context 'when the file path fits the known pattern' do
+          let!(:orphaned) { create(:upload, :issuable_upload, :with_file, model: build(:project, :legacy_storage)) }
+          let!(:path) { orphaned.absolute_path }
+          let!(:new_path) { File.join(FileUploader.root, '-', 'project-lost-found', orphaned.model.full_path, orphaned.path) }
+
+          before do
+            orphaned.delete
+          end
+
+          it_behaves_like 'moves the file to lost and found'
+        end
+
+        context 'when the file path does not fit the known pattern' do
+          let!(:invalid_path) { File.join('group', 'file.jpg') }
+          let!(:path) { File.join(FileUploader.root, invalid_path) }
+          let!(:new_path) { File.join(FileUploader.root, '-', 'project-lost-found', invalid_path) }
+
+          before do
+            FileUtils.mkdir_p(File.dirname(path))
+            FileUtils.touch(path)
+          end
+
+          after do
+            File.delete(path) if File.exist?(path)
+          end
+
+          it_behaves_like 'moves the file to lost and found'
+        end
+      end
+    end
+
+    context 'non-orphaned project upload file' do
+      it 'does not move the file' do
+        tracked = create(:upload, :issuable_upload, :with_file, model: build(:project, :legacy_storage))
+        tracked_path = tracked.absolute_path
+
+        expect(logger).not_to receive(:info).with(/move|fix/i)
+        expect(File.exist?(tracked_path)).to be_truthy
+
+        subject.run!(dry_run: false)
+
+        expect(File.exist?(tracked_path)).to be_truthy
+      end
+    end
+
+    context 'ignorable cases' do
+      # Because we aren't concerned about these, and can save a lot of
+      # processing time by ignoring them. If we wish to cleanup hashed storage
+      # directories, it should simply require removing this test and modifying
+      # the find command.
+      context 'when the file is already in hashed storage' do
+        let(:project) { create(:project) }
+
+        before do
+          expect(logger).not_to receive(:info).with(/move|fix/i)
+        end
+
+        it 'does not move even an orphan file' do
+          orphaned = create(:upload, :issuable_upload, :with_file, model: project)
+          path = orphaned.absolute_path
+          orphaned.delete
+
+          expect(File.exist?(path)).to be_truthy
+
+          subject.run!(dry_run: false)
+
+          expect(File.exist?(path)).to be_truthy
+        end
+      end
+
+      it 'does not move any non-project (FileUploader) uploads' do
+        paths = []
+        orphaned1 = create(:upload, :personal_snippet_upload, :with_file)
+        orphaned2 = create(:upload, :namespace_upload, :with_file)
+        orphaned3 = create(:upload, :attachment_upload, :with_file)
+        paths << orphaned1.absolute_path
+        paths << orphaned2.absolute_path
+        paths << orphaned3.absolute_path
+        Upload.delete_all
+
+        expect(logger).not_to receive(:info).with(/move|fix/i)
+        paths.each do |path|
+          expect(File.exist?(path)).to be_truthy
+        end
+
+        subject.run!(dry_run: false)
+
+        paths.each do |path|
+          expect(File.exist?(path)).to be_truthy
+        end
+      end
+
+      it 'does not move any uploads in tmp (which would interfere with ongoing upload activity)' do
+        path = File.join(FileUploader.root, 'tmp', 'foo.jpg')
+        FileUtils.mkdir_p(File.dirname(path))
+        FileUtils.touch(path)
+
+        expect(logger).not_to receive(:info).with(/move|fix/i)
+        expect(File.exist?(path)).to be_truthy
+
+        subject.run!(dry_run: false)
+
+        expect(File.exist?(path)).to be_truthy
+      end
+    end
+  end
+end

spec/tasks/gitlab/cleanup_rake_spec.rb

@@ -68,317 +68,86 @@ describe 'gitlab:cleanup rake tasks' do
     end
   end
 
+  # A single integration test that is redundant with one part of the
+  # Gitlab::Cleanup::ProjectUploads spec.
+  #
+  # Additionally, this tests DRY_RUN env var values, and the extra line of
+  # output that says you can disable DRY_RUN if it's enabled.
   describe 'cleanup:project_uploads' do
-    context 'orphaned project upload file' do
-      context 'when an upload record matching the secret and filename is found' do
-        context 'when the project is still in legacy storage' do
-          let!(:orphaned) { create(:upload, :issuable_upload, :with_file, model: build(:project, :legacy_storage)) }
-          let!(:correct_path) { orphaned.absolute_path }
-          let!(:other_project) { create(:project, :legacy_storage) }
-          let!(:orphaned_path) { correct_path.sub(/#{orphaned.model.full_path}/, other_project.full_path) }
+    let!(:logger) { double(:logger) }
 
     before do
-            FileUtils.mkdir_p(File.dirname(orphaned_path))
-            FileUtils.mv(correct_path, orphaned_path)
-          end
-
-          it 'moves the file to its proper location' do
-            expect(Rails.logger).to receive(:info).twice
-            expect(Rails.logger).to receive(:info).with("Did fix #{orphaned_path} -> #{correct_path}")
-
-            expect(File.exist?(orphaned_path)).to be_truthy
-            expect(File.exist?(correct_path)).to be_falsey
+      expect(main_object).to receive(:logger).and_return(logger).at_least(1).times
 
-            stub_env('DRY_RUN', 'false')
-            run_rake_task('gitlab:cleanup:project_uploads')
-
-            expect(File.exist?(orphaned_path)).to be_falsey
-            expect(File.exist?(correct_path)).to be_truthy
+      allow(logger).to receive(:info).at_least(1).times
+      allow(logger).to receive(:debug).at_least(1).times
     end
 
-          it 'a dry run does not move the file' do
-            expect(Rails.logger).to receive(:info).twice
-            expect(Rails.logger).to receive(:info).with("Can fix #{orphaned_path} -> #{correct_path}")
-            expect(Rails.logger).to receive(:info)
-
-            expect(File.exist?(orphaned_path)).to be_truthy
-            expect(File.exist?(correct_path)).to be_falsey
-
-            run_rake_task('gitlab:cleanup:project_uploads')
-
-            expect(File.exist?(orphaned_path)).to be_truthy
-            expect(File.exist?(correct_path)).to be_falsey
-          end
-
-          context 'when the project record is missing (Upload#absolute_path raises error)' do
-            let!(:lost_and_found_path) { File.join(FileUploader.root, '-', 'project-lost-found', other_project.full_path, orphaned.path) }
+    context 'with a fixable orphaned project upload file' do
+      let(:orphaned) { create(:upload, :issuable_upload, :with_file, model: build(:project, :legacy_storage)) }
+      let(:new_path) { orphaned.absolute_path }
+      let(:path) { File.join(FileUploader.root, 'some', 'wrong', 'location', orphaned.path) }
 
       before do
-              orphaned.model.delete
-            end
-
-            it 'moves the file to lost and found' do
-              expect(Rails.logger).to receive(:info).twice
-              expect(Rails.logger).to receive(:info).with("Did move to lost and found #{orphaned_path} -> #{lost_and_found_path}")
-
-              expect(File.exist?(orphaned_path)).to be_truthy
-              expect(File.exist?(lost_and_found_path)).to be_falsey
-
-              stub_env('DRY_RUN', 'false')
-              run_rake_task('gitlab:cleanup:project_uploads')
-
-              expect(File.exist?(orphaned_path)).to be_falsey
-              expect(File.exist?(lost_and_found_path)).to be_truthy
-            end
-
-            it 'a dry run does not move the file' do
-              expect(Rails.logger).to receive(:info).twice
-              expect(Rails.logger).to receive(:info).with("Can move to lost and found #{orphaned_path} -> #{lost_and_found_path}")
-              expect(Rails.logger).to receive(:info)
-
-              expect(File.exist?(orphaned_path)).to be_truthy
-              expect(File.exist?(lost_and_found_path)).to be_falsey
-
-              run_rake_task('gitlab:cleanup:project_uploads')
-
-              expect(File.exist?(orphaned_path)).to be_truthy
-              expect(File.exist?(lost_and_found_path)).to be_falsey
-            end
-          end
-        end
-
-        context 'when the project was moved to hashed storage' do
-          let!(:orphaned) { create(:upload, :issuable_upload, :with_file) }
-          let!(:correct_path) { orphaned.absolute_path }
-          let!(:orphaned_path) { File.join(FileUploader.root, 'foo', 'bar', orphaned.path) }
-
-          before do
-            FileUtils.mkdir_p(File.dirname(orphaned_path))
-            FileUtils.mv(correct_path, orphaned_path)
-          end
-
-          it 'moves the file to its proper location' do
-            expect(Rails.logger).to receive(:info).twice
-            expect(Rails.logger).to receive(:info).with("Did fix #{orphaned_path} -> #{correct_path}")
-
-            expect(File.exist?(orphaned_path)).to be_truthy
-            expect(File.exist?(correct_path)).to be_falsey
-
-            stub_env('DRY_RUN', 'false')
-            run_rake_task('gitlab:cleanup:project_uploads')
-
-            expect(File.exist?(orphaned_path)).to be_falsey
-            expect(File.exist?(correct_path)).to be_truthy
-          end
-
-          it 'a dry run does not move the file' do
-            expect(Rails.logger).to receive(:info).twice
-            expect(Rails.logger).to receive(:info).with("Can fix #{orphaned_path} -> #{correct_path}")
-            expect(Rails.logger).to receive(:info)
-
-            expect(File.exist?(orphaned_path)).to be_truthy
-            expect(File.exist?(correct_path)).to be_falsey
-
-            run_rake_task('gitlab:cleanup:project_uploads')
-
-            expect(File.exist?(orphaned_path)).to be_truthy
-            expect(File.exist?(correct_path)).to be_falsey
-          end
-        end
+        FileUtils.mkdir_p(File.dirname(path))
+        FileUtils.mv(new_path, path)
       end
 
-      context 'when a matching upload record can not be found' do
-        context 'when the file path fits the known pattern' do
-          let!(:orphaned) { create(:upload, :issuable_upload, :with_file, model: build(:project, :legacy_storage)) }
-          let!(:orphaned_path) { orphaned.absolute_path }
-          let!(:lost_and_found_path) { File.join(FileUploader.root, '-', 'project-lost-found', orphaned.model.full_path, orphaned.path) }
-
+      context 'with DRY_RUN disabled' do
         before do
-            orphaned.delete
-          end
-
-          it 'moves the file to lost and found' do
-            expect(Rails.logger).to receive(:info).twice
-            expect(Rails.logger).to receive(:info).with("Did move to lost and found #{orphaned_path} -> #{lost_and_found_path}")
-
-            expect(File.exist?(orphaned_path)).to be_truthy
-            expect(File.exist?(lost_and_found_path)).to be_falsey
-
           stub_env('DRY_RUN', 'false')
-            run_rake_task('gitlab:cleanup:project_uploads')
-
-            expect(File.exist?(orphaned_path)).to be_falsey
-            expect(File.exist?(lost_and_found_path)).to be_truthy
         end
 
-          it 'a dry run does not move the file' do
-            expect(Rails.logger).to receive(:info).twice
-            expect(Rails.logger).to receive(:info).with("Can move to lost and found #{orphaned_path} -> #{lost_and_found_path}")
-            expect(Rails.logger).to receive(:info)
-
-            expect(File.exist?(orphaned_path)).to be_truthy
-            expect(File.exist?(lost_and_found_path)).to be_falsey
-
-            run_rake_task('gitlab:cleanup:project_uploads')
-
-            expect(File.exist?(orphaned_path)).to be_truthy
-            expect(File.exist?(lost_and_found_path)).to be_falsey
-          end
-        end
-
-        context 'when the file path does not fit the known pattern' do
-          let!(:invalid_path) { File.join('group', 'file.jpg') }
-          let!(:orphaned_path) { File.join(FileUploader.root, invalid_path) }
-          let!(:lost_and_found_path) { File.join(FileUploader.root, '-', 'project-lost-found', invalid_path) }
-
-          before do
-            FileUtils.mkdir_p(File.dirname(orphaned_path))
-            FileUtils.touch(orphaned_path)
-          end
-
-          after do
-            File.delete(orphaned_path) if File.exist?(orphaned_path)
-          end
-
-          it 'moves the file to lost and found' do
-            expect(Rails.logger).to receive(:info).twice
-            expect(Rails.logger).to receive(:info).with("Did move to lost and found #{orphaned_path} -> #{lost_and_found_path}")
-
-            expect(File.exist?(orphaned_path)).to be_truthy
-            expect(File.exist?(lost_and_found_path)).to be_falsey
-
-            stub_env('DRY_RUN', 'false')
+        it 'moves the file to its proper location' do
           run_rake_task('gitlab:cleanup:project_uploads')
 
-            expect(File.exist?(orphaned_path)).to be_falsey
-            expect(File.exist?(lost_and_found_path)).to be_truthy
+          expect(File.exist?(path)).to be_falsey
+          expect(File.exist?(new_path)).to be_truthy
         end
 
-          it 'a dry run does not move the file' do
-            expect(Rails.logger).to receive(:info).twice
-            expect(Rails.logger).to receive(:info).with("Can move to lost and found #{orphaned_path} -> #{lost_and_found_path}")
-            expect(Rails.logger).to receive(:info)
-
-            expect(File.exist?(orphaned_path)).to be_truthy
-            expect(File.exist?(lost_and_found_path)).to be_falsey
+        it 'logs action as done' do
+          expect(logger).to receive(:info).with("Looking for orphaned project uploads to clean up...")
+          expect(logger).to receive(:info).with("Did fix #{path} -> #{new_path}")
 
           run_rake_task('gitlab:cleanup:project_uploads')
-
-            expect(File.exist?(orphaned_path)).to be_truthy
-            expect(File.exist?(lost_and_found_path)).to be_falsey
-          end
-        end
         end
       end
 
-    context 'non-orphaned project upload file' do
+      shared_examples_for 'does not move the file' do
         it 'does not move the file' do
-        tracked = create(:upload, :issuable_upload, :with_file, model: build(:project, :legacy_storage))
-        tracked_path = tracked.absolute_path
-
-        expect(Rails.logger).not_to receive(:info).with(/move|fix/i)
-        expect(File.exist?(tracked_path)).to be_truthy
-
-        stub_env('DRY_RUN', 'false')
           run_rake_task('gitlab:cleanup:project_uploads')
 
-        expect(File.exist?(tracked_path)).to be_truthy
-      end
+          expect(File.exist?(path)).to be_truthy
+          expect(File.exist?(new_path)).to be_falsey
         end
 
-    context 'ignorable cases' do
-      shared_examples_for 'does not move anything' do
-        it 'does not move even an orphan file' do
-          orphaned = create(:upload, :issuable_upload, :with_file, model: project)
-          orphaned_path = orphaned.absolute_path
-          orphaned.delete
-
-          expect(File.exist?(orphaned_path)).to be_truthy
+        it 'logs action as able to be done' do
+          expect(logger).to receive(:info).with("Looking for orphaned project uploads to clean up. Dry run...")
+          expect(logger).to receive(:info).with("Can fix #{path} -> #{new_path}")
+          expect(logger).to receive(:info).with(/To clean up these files run this command with DRY_RUN=false/)
 
           run_rake_task('gitlab:cleanup:project_uploads')
-
-          expect(File.exist?(orphaned_path)).to be_truthy
-        end
-      end
-
-      # Because we aren't concerned about these, and can save a lot of
-      # processing time by ignoring them. If we wish to cleanup hashed storage
-      # directories, it should simply require removing this test and modifying
-      # the find command.
-      context 'when the file is already in hashed storage' do
-        let(:project) { create(:project) }
-
-        before do
-          stub_env('DRY_RUN', 'false')
-          expect(Rails.logger).not_to receive(:info).with(/move|fix/i)
-        end
-
-        it_behaves_like 'does not move anything'
         end
-
-      context 'when DRY_RUN env var is unset' do
-        let(:project) { create(:project, :legacy_storage) }
-
-        it_behaves_like 'does not move anything'
       end
 
-      context 'when DRY_RUN env var is true' do
-        let(:project) { create(:project, :legacy_storage) }
-
+      context 'with DRY_RUN explicitly enabled' do
         before do
           stub_env('DRY_RUN', 'true')
         end
 
-        it_behaves_like 'does not move anything'
+        it_behaves_like 'does not move the file'
       end
 
-      context 'when DRY_RUN env var is foo' do
-        let(:project) { create(:project, :legacy_storage) }
-
+      context 'with DRY_RUN set to an unknown value' do
         before do
           stub_env('DRY_RUN', 'foo')
         end
 
-        it_behaves_like 'does not move anything'
+        it_behaves_like 'does not move the file'
       end
 
-      it 'does not move any non-project (FileUploader) uploads' do
-        stub_env('DRY_RUN', 'false')
-
-        paths = []
-        orphaned1 = create(:upload, :personal_snippet_upload, :with_file)
-        orphaned2 = create(:upload, :namespace_upload, :with_file)
-        orphaned3 = create(:upload, :attachment_upload, :with_file)
-        paths << orphaned1.absolute_path
-        paths << orphaned2.absolute_path
-        paths << orphaned3.absolute_path
-        Upload.delete_all
-
-        expect(Rails.logger).not_to receive(:info).with(/move|fix/i)
-        paths.each do |path|
-          expect(File.exist?(path)).to be_truthy
-        end
-
-        run_rake_task('gitlab:cleanup:project_uploads')
-
-        paths.each do |path|
-          expect(File.exist?(path)).to be_truthy
-        end
-      end
-
-      it 'does not move any uploads in tmp (which would interfere with ongoing upload activity)' do
-        stub_env('DRY_RUN', 'false')
-
-        path = File.join(FileUploader.root, 'tmp', 'foo.jpg')
-        FileUtils.mkdir_p(File.dirname(path))
-        FileUtils.touch(path)
-
-        expect(Rails.logger).not_to receive(:info).with(/move|fix/i)
-        expect(File.exist?(path)).to be_truthy
-
-        run_rake_task('gitlab:cleanup:project_uploads')
-
-        expect(File.exist?(path)).to be_truthy
+      context 'with DRY_RUN unset' do
+        it_behaves_like 'does not move the file'
       end
     end
   end