diff options
author | Stan Hu <stanhu@gmail.com> | 2018-08-03 04:36:43 +0000 |
---|---|---|
committer | Stan Hu <stanhu@gmail.com> | 2018-08-03 04:36:43 +0000 |
commit | 5f664759b57ef1c0fcfb7e95dfbff6c080a7a72f (patch) | |
tree | 74c2f0d6c7aa0a271b91c097c6703d5f0ab69e0a | |
parent | f58122960aa5efbb9420ba4b092de4b5a3de2ae9 (diff) | |
parent | a4351ac077b9f266b4bcfa8f1c4867a837870a27 (diff) | |
download | gitlab-ce-expose-secondary-emails-api.tar.gz |
Merge branch 'mk/add-orphan-upload-file-task-tests' into 'master'expose-secondary-emails-api
Add object storage related tests for `gitlab:cleanup:project_uploads` task
See merge request gitlab-org/gitlab-ce!20950
-rw-r--r-- | lib/gitlab/cleanup/project_uploads.rb | 2 | ||||
-rw-r--r-- | spec/lib/gitlab/cleanup/project_uploads_spec.rb | 278 | ||||
-rw-r--r-- | spec/tasks/gitlab/cleanup_rake_spec.rb | 321 |
3 files changed, 324 insertions, 277 deletions
diff --git a/lib/gitlab/cleanup/project_uploads.rb b/lib/gitlab/cleanup/project_uploads.rb index b88e00311d5..f55ab535efe 100644 --- a/lib/gitlab/cleanup/project_uploads.rb +++ b/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 diff --git a/spec/lib/gitlab/cleanup/project_uploads_spec.rb b/spec/lib/gitlab/cleanup/project_uploads_spec.rb new file mode 100644 index 00000000000..37b38776775 --- /dev/null +++ b/spec/lib/gitlab/cleanup/project_uploads_spec.rb @@ -0,0 +1,278 @@ +# 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 diff --git a/spec/tasks/gitlab/cleanup_rake_spec.rb b/spec/tasks/gitlab/cleanup_rake_spec.rb index ba08ece1b4b..cc2cca10f58 100644 --- a/spec/tasks/gitlab/cleanup_rake_spec.rb +++ b/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 - - 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 - - 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) } - - 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) + before do + expect(main_object).to receive(:logger).and_return(logger).at_least(1).times - expect(File.exist?(orphaned_path)).to be_truthy - expect(File.exist?(correct_path)).to be_falsey + allow(logger).to receive(:info).at_least(1).times + allow(logger).to receive(:debug).at_least(1).times + end - run_rake_task('gitlab:cleanup:project_uploads') + 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) } - expect(File.exist?(orphaned_path)).to be_truthy - expect(File.exist?(correct_path)).to be_falsey - end - end + before do + 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) } - - 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 + context 'with DRY_RUN disabled' do + before do + stub_env('DRY_RUN', 'false') 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') - 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') + it 'moves the file to its proper location' do + 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 + expect(File.exist?(path)).to be_falsey + expect(File.exist?(new_path)).to be_truthy 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(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 - 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 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 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) } + shared_examples_for 'does not move the file' do + it 'does not move the file' do + run_rake_task('gitlab:cleanup:project_uploads') - before do - stub_env('DRY_RUN', 'false') - expect(Rails.logger).not_to receive(:info).with(/move|fix/i) + expect(File.exist?(path)).to be_truthy + expect(File.exist?(new_path)).to be_falsey 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 '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/) - it_behaves_like 'does not move anything' + run_rake_task('gitlab:cleanup:project_uploads') + end 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 |