diff --git a/doc/administration/cicd/job_artifacts.md b/doc/administration/cicd/job_artifacts.md index b2f6aafad0d232d8ddffbd820f7a24ba59da84ad..f4c4f0736ebd2a7fd1fe7e7deeaad9741766eaad 100644 --- a/doc/administration/cicd/job_artifacts.md +++ b/doc/administration/cicd/job_artifacts.md @@ -324,6 +324,10 @@ To migrate artifacts back to local storage: 1. [Selectively disable the artifacts' storage](../object_storage.md#disable-object-storage-for-specific-features) in `gitlab.rb`. 1. [Reconfigure GitLab](../restart_gitlab.md#reconfigure-a-linux-package-installation). +Until GitLab 18.6, a migration from remote to local storage could result in artifacts being copied with incorrect filenames. +If this has happened to you, please run `gitlab-rake gitlab:artifacts:fix_artifact_filepath`. +This task will check for artifacts that are not downloadable and will rename them in your local storage to the appropriate, expected file name. + ## Expiring artifacts If [`artifacts:expire_in`](../../ci/yaml/_index.md#artifactsexpire_in) is used to set diff --git a/lib/gitlab/local_and_remote_storage_migration/artifact_local_storage_name_fixer.rb b/lib/gitlab/local_and_remote_storage_migration/artifact_local_storage_name_fixer.rb new file mode 100644 index 0000000000000000000000000000000000000000..3feffd4082f510f1d33d3298bc0eac3f5319a356 --- /dev/null +++ b/lib/gitlab/local_and_remote_storage_migration/artifact_local_storage_name_fixer.rb @@ -0,0 +1,42 @@ +# frozen_string_literal: true + +module Gitlab + module LocalAndRemoteStorageMigration + class ArtifactLocalStorageNameFixer + def initialize(logger = nil) + @logger = logger + end + + def rename_artifacts + logger.info('Starting renaming process in local storage') + items_with_files_stored_locally.each_batch(of: batch_size) do |batch| + batch.each do |item| + log_success(item) if FilePathFixer.fix_file_path!(item) + rescue NoMethodError, ArgumentError, Errno::ENOENT, Errno::EACCES, Errno::ENOTDIR, Errno::EEXIST, IOError => e + log_error(e, item) + end + end + end + + private + + attr_reader :logger + + def batch_size + ENV.fetch('MIGRATION_BATCH_SIZE', 10).to_i + end + + def log_success(item) + logger.info("Renamed #{item.class.name} ID #{item.id} with size #{item.size}.") + end + + def log_error(err, item) + logger.warn("Failed to rename #{item.class.name} ID #{item.id} with error: #{err.message}.") + end + + def items_with_files_stored_locally + ::Ci::JobArtifact.with_files_stored_locally + end + end + end +end diff --git a/lib/gitlab/local_and_remote_storage_migration/artifact_migrater.rb b/lib/gitlab/local_and_remote_storage_migration/artifact_migrater.rb index b25305382b2deeb15855cea713284b47339d02cd..4dd40c3fbf1253b049965aeeb16ee75c569ed658 100644 --- a/lib/gitlab/local_and_remote_storage_migration/artifact_migrater.rb +++ b/lib/gitlab/local_and_remote_storage_migration/artifact_migrater.rb @@ -5,6 +5,20 @@ module LocalAndRemoteStorageMigration class ArtifactMigrater < Gitlab::LocalAndRemoteStorageMigration::BaseMigrater private + def migrate(items, store) + items.each_batch(of: batch_size) do |batch| + batch.each do |item| + item.file.migrate!(store) + + FilePathFixer.fix_file_path!(item) if store == ObjectStorage::Store::LOCAL + + log_success(item, store) + rescue NoMethodError, ArgumentError, Errno::ENOENT, Errno::EACCES, Errno::ENOTDIR, Errno::EEXIST, IOError => e + log_error(e, item) + end + end + end + def items_with_files_stored_locally ::Ci::JobArtifact.with_files_stored_locally end diff --git a/lib/gitlab/local_and_remote_storage_migration/file_path_fixer.rb b/lib/gitlab/local_and_remote_storage_migration/file_path_fixer.rb new file mode 100644 index 0000000000000000000000000000000000000000..c92de2001ac55752a0a915c40fe437549f21f96e --- /dev/null +++ b/lib/gitlab/local_and_remote_storage_migration/file_path_fixer.rb @@ -0,0 +1,29 @@ +# frozen_string_literal: true + +module Gitlab + module LocalAndRemoteStorageMigration + module FilePathFixer + extend self + + # Fixes the file path if necessary and returns the new path (or nil if nothing changed) + def fix_file_path!(artifact) + return if artifact.file_final_path.blank? + + desired_file_name = artifact.attributes['file'] + final_file_dir = File.dirname(artifact.file.path) + remote_file_name = File.basename(artifact.file_final_path) + + return if desired_file_name == remote_file_name + + return if File.exist?(File.join(final_file_dir, desired_file_name)) + + to_be_renamed_file = File.join(final_file_dir, remote_file_name) + return unless File.exist?(to_be_renamed_file) + + final_file_name = File.join(final_file_dir, desired_file_name) + File.rename(to_be_renamed_file, final_file_name) + final_file_name + end + end + end +end diff --git a/lib/tasks/gitlab/artifacts/fix_artifact_filepath.rake b/lib/tasks/gitlab/artifacts/fix_artifact_filepath.rake new file mode 100644 index 0000000000000000000000000000000000000000..e1c368d97994e22259e70d0e8c2202811964835f --- /dev/null +++ b/lib/tasks/gitlab/artifacts/fix_artifact_filepath.rake @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +desc 'GitLab | Artifacts | Fix uploaded filenames for artifacts on local storage by renaming them appropriately' +namespace :gitlab do + require 'logger' # rubocop:disable Rake/Require -- we need to load the logger + + namespace :artifacts do + task fix_artifact_filepath: :environment do + require 'resolv-replace' + logger = Logger.new($stdout) + + helper = Gitlab::LocalAndRemoteStorageMigration::ArtifactLocalStorageNameFixer.new(logger) + + helper.rename_artifacts + end + end +end diff --git a/spec/lib/gitlab/local_and_remote_storage_migration/artifact_local_storage_name_fixer_spec.rb b/spec/lib/gitlab/local_and_remote_storage_migration/artifact_local_storage_name_fixer_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..fbe937292847fa8cf8a6627b0baa47593d869d83 --- /dev/null +++ b/spec/lib/gitlab/local_and_remote_storage_migration/artifact_local_storage_name_fixer_spec.rb @@ -0,0 +1,84 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::LocalAndRemoteStorageMigration::ArtifactLocalStorageNameFixer, feature_category: :job_artifacts do + let(:logger) { instance_double(Logger, info: nil, warn: nil) } + let(:fixer) { described_class.new(logger) } + let(:artifact_class) { class_double(Ci::JobArtifact) } + + before do + stub_const('Ci::JobArtifact', artifact_class) + end + + describe '#rename_artifacts' do + let(:artifact1) do + instance_double(Ci::JobArtifact, id: 1, size: 123, class: class_double(Ci::JobArtifact, name: 'Ci::JobArtifact')) + end + + let(:artifact2) do + instance_double(Ci::JobArtifact, id: 2, size: 456, class: class_double(Ci::JobArtifact, name: 'Ci::JobArtifact')) + end + + let(:batch_relation_class) do + Class.new do + include EachBatch + end + end + + let(:batch_relation) { class_double(batch_relation_class, each_batch: nil) } + + before do + allow(artifact_class).to receive(:with_files_stored_locally).and_return(batch_relation) + allow(batch_relation).to receive(:each_batch).and_yield([artifact1, artifact2]) + end + + it 'logs start message' do + expect(logger).to receive(:info).with('Starting renaming process in local storage') + allow(Gitlab::LocalAndRemoteStorageMigration::FilePathFixer).to receive(:fix_file_path!).and_return(true) + + fixer.rename_artifacts + end + + it 'calls FilePathFixer for each artifact' do + expect(Gitlab::LocalAndRemoteStorageMigration::FilePathFixer) + .to receive(:fix_file_path!).with(artifact1).and_return(true) + expect(Gitlab::LocalAndRemoteStorageMigration::FilePathFixer) + .to receive(:fix_file_path!).with(artifact2).and_return(true) + + fixer.rename_artifacts + end + + it 'logs successful renames' do + allow(Gitlab::LocalAndRemoteStorageMigration::FilePathFixer) + .to receive(:fix_file_path!).and_return(true) + + expect(logger).to receive(:info).with('Starting renaming process in local storage') + expect(logger).to receive(:info).with('Renamed Ci::JobArtifact ID 1 with size 123.') + expect(logger).to receive(:info).with('Renamed Ci::JobArtifact ID 2 with size 456.') + + fixer.rename_artifacts + end + + it 'logs warnings on errors' do + allow(Gitlab::LocalAndRemoteStorageMigration::FilePathFixer) + .to receive(:fix_file_path!).and_raise(IOError, 'boom') + + expect(logger).to receive(:warn).with('Failed to rename Ci::JobArtifact ID 1 with error: boom.') + expect(logger).to receive(:warn).with('Failed to rename Ci::JobArtifact ID 2 with error: boom.') + + fixer.rename_artifacts + end + end + + describe '#batch_size' do + it 'returns default value of 10' do + expect(fixer.send(:batch_size)).to eq(10) + end + + it 'returns custom value from ENV' do + stub_env('MIGRATION_BATCH_SIZE', '50') + expect(fixer.send(:batch_size)).to eq(50) + end + end +end diff --git a/spec/lib/gitlab/local_and_remote_storage_migration/artifact_migrater_spec.rb b/spec/lib/gitlab/local_and_remote_storage_migration/artifact_migrater_spec.rb index b3f2003c207ea8daf18b9217a39d241ad699ea1e..cc92d5704182475f10d7bf33ab65fc3478fc08b0 100644 --- a/spec/lib/gitlab/local_and_remote_storage_migration/artifact_migrater_spec.rb +++ b/spec/lib/gitlab/local_and_remote_storage_migration/artifact_migrater_spec.rb @@ -10,5 +10,5 @@ let!(:item) { create(:ci_job_artifact, :archive, file_store: start_store) } - it_behaves_like 'local and remote storage migration' + it_behaves_like 'local and remote storage migration', expect_file_rename: true end diff --git a/spec/lib/gitlab/local_and_remote_storage_migration/file_path_fixer_spec.rb b/spec/lib/gitlab/local_and_remote_storage_migration/file_path_fixer_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..e1ee111556b6eac8068057b96c07ba7e8fee6392 --- /dev/null +++ b/spec/lib/gitlab/local_and_remote_storage_migration/file_path_fixer_spec.rb @@ -0,0 +1,99 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::LocalAndRemoteStorageMigration::FilePathFixer, feature_category: :job_artifacts do + let(:artifact) do + instance_double( + Ci::JobArtifact, + file_final_path: file_final_path, + file: uploader_double, + attributes: { 'file' => 'desired_name.zip' } + ) + end + + let(:uploader_double) do + instance_double( + JobArtifactUploader, + path: local_path + ) + end + + let(:desired_file_name) { 'desired_name.zip' } + let(:remote_file_name) { 'remote_name.zip' } + let(:local_path) { "/local/path/#{remote_file_name}" } + let(:file_final_path) { "/remote/path/#{remote_file_name}" } + + let(:renamed_files) { [] } + + before do + allow(artifact).to receive(:[]).with('file').and_return(desired_file_name) + allow(artifact.file).to receive(:path).and_return("/local/path/#{remote_file_name}") + + allow(File).to receive(:exist?).and_call_original + allow(File).to receive(:exist?).with("/local/path/#{desired_file_name}").and_return(desired_exists) + allow(File).to receive(:exist?).with(local_path).and_return(remote_exists) + + allow(File).to receive(:rename) do |old_path, new_path| + renamed_files << [old_path, new_path] + end + end + + describe '.fix_file_path!' do + subject(:fix_file_path) { described_class.fix_file_path!(artifact) } + + context 'when file_final_path is blank' do + let(:file_final_path) { nil } + let(:desired_exists) { false } + let(:remote_exists) { true } + + it 'does nothing' do + expect(fix_file_path).to be_nil + expect(renamed_files).to be_empty + end + end + + context 'when desired file name matches remote file name' do + let(:desired_file_name) { 'desired_name.zip' } + let(:remote_file_name) { 'desired_name.zip' } + + let(:desired_exists) { false } + let(:remote_exists) { true } + + it 'does nothing' do + expect(fix_file_path).to be_nil + expect(renamed_files).to be_empty + end + end + + context 'when desired file already exists locally' do + let(:desired_exists) { true } + let(:remote_exists) { true } + + it 'does nothing' do + expect(fix_file_path).to be_nil + expect(renamed_files).to be_empty + end + end + + context 'when file needs to be renamed' do + let(:desired_exists) { false } + let(:remote_exists) { true } + + it 'renames the file' do + is_expected.to eq("/local/path/#{desired_file_name}") + expect(renamed_files).to match_array([[local_path, "/local/path/#{desired_file_name}"]]) + end + end + + context 'when remote file does not exist' do + let(:desired_exists) { false } + let(:remote_exists) { false } + + it 'does nothing' do + expect(fix_file_path).to be_nil + expect(renamed_files).to be_empty + end + end + end +end diff --git a/spec/support/shared_examples/lib/gitlab/local_and_remote_storage_migration_shared_examples.rb b/spec/support/shared_examples/lib/gitlab/local_and_remote_storage_migration_shared_examples.rb index 4b0e3234750f228f8ed1d473a1914f5238a827e6..8f8a0822cf3213beaa4744e3705847d843da6f50 100644 --- a/spec/support/shared_examples/lib/gitlab/local_and_remote_storage_migration_shared_examples.rb +++ b/spec/support/shared_examples/lib/gitlab/local_and_remote_storage_migration_shared_examples.rb @@ -1,6 +1,6 @@ # frozen_string_literal: true -RSpec.shared_examples 'local and remote storage migration' do +RSpec.shared_examples 'local and remote storage migration' do |expect_file_rename: false| let(:logger) { Logger.new("/dev/null") } let(:migrater) { described_class.new(logger) } @@ -25,6 +25,10 @@ expect(item.file_store).to eq(start_store) + if expect_file_rename && start_store == ObjectStorage::Store::REMOTE && end_store == ObjectStorage::Store::LOCAL + expect(Gitlab::LocalAndRemoteStorageMigration::FilePathFixer).to receive(:fix_file_path!) + end + migrater.send(method) expect(item.reload.file_store).to eq(end_store) @@ -36,7 +40,7 @@ it 'prints error' do expect_next_instance_of(item.file.class) do |file| - expect(file).to receive(:migrate!).and_raise("error message") + expect(file).to receive(:migrate!).and_raise(IOError, "error message") end expect(logger).to receive(:info).with("Starting transfer to object storage") diff --git a/spec/tasks/gitlab/artifacts/fix_artifact_filepath_spec.rb b/spec/tasks/gitlab/artifacts/fix_artifact_filepath_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..3e5ef78db0c528eb683cf3fcd60e006dcccef4c5 --- /dev/null +++ b/spec/tasks/gitlab/artifacts/fix_artifact_filepath_spec.rb @@ -0,0 +1,72 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe 'gitlab:artifacts namespace rake task', :silence_stdout, feature_category: :job_artifacts do + before(:context) do + Rake.application.rake_require 'tasks/gitlab/artifacts/fix_artifact_filepath' + end + + let(:desired_file_name) { 'desired_name.zip' } + let(:remote_file_name) { 'remote_name.zip' } + let(:local_path) { "/local/path/#{remote_file_name}" } + let(:file_final_path) { "/remote/path/#{remote_file_name}" } + + let(:renamed_files) { [] } + let(:relation_class) do + Class.new do + include EachBatch + end + end + + let(:relation) { class_double(relation_class, each_batch: nil) } + + let(:artifact) do + instance_double( + Ci::JobArtifact, + file_final_path: file_final_path, + file: uploader_double, + attributes: { 'file' => 'desired_name.zip' }, + id: 1, + size: 0 + ) + end + + let(:uploader_double) do + instance_double( + JobArtifactUploader, + path: local_path + ) + end + + before do + allow(artifact).to receive(:[]).with('file').and_return(desired_file_name) + allow(artifact.file).to receive(:path).and_return("/local/path/#{remote_file_name}") + + allow(File).to receive(:exist?).and_call_original + allow(File).to receive(:exist?).with("/local/path/#{desired_file_name}").and_return(desired_exists) + allow(File).to receive(:exist?).with(local_path).and_return(remote_exists) + + allow(File).to receive(:rename) do |old_path, new_path| + renamed_files << [old_path, new_path] + end + + allow(relation).to receive(:each_batch).and_yield([artifact]) + + allow(Ci::JobArtifact).to receive(:with_files_stored_locally).and_return(relation) + end + + describe 'gitlab:artifacts:migrate' do + subject(:task) { run_rake_task('gitlab:artifacts:fix_artifact_filepath') } + + context 'when file needs to be renamed' do + let(:desired_exists) { false } + let(:remote_exists) { true } + + it 'renames the file' do + task + expect(renamed_files).to match_array([[local_path, "/local/path/#{desired_file_name}"]]) + end + end + end +end