diff options
author | Kamil Trzcinski <ayufan@ayufan.eu> | 2017-05-04 23:02:51 +0200 |
---|---|---|
committer | Kamil Trzcinski <ayufan@ayufan.eu> | 2017-06-01 14:39:48 +0200 |
commit | e61f38d79eb85a7c601bd146d5b8e48a8b4418e5 (patch) | |
tree | 08abc6cb3f4ef57de157167fca8189a162298ae1 | |
parent | c72abcefe79dd906cbbf0088b442a8979e9fc746 (diff) | |
download | gitlab-ce-e61f38d79eb85a7c601bd146d5b8e48a8b4418e5.tar.gz |
Fix data inconsistency issue for old artifacts by moving them to a currently used path
-rw-r--r-- | app/controllers/projects/artifacts_controller.rb | 2 | ||||
-rw-r--r-- | app/controllers/uploads_controller.rb | 2 | ||||
-rw-r--r-- | app/models/ci/build.rb | 32 | ||||
-rw-r--r-- | app/uploaders/artifact_uploader.rb | 28 | ||||
-rw-r--r-- | app/uploaders/gitlab_uploader.rb | 12 | ||||
-rw-r--r-- | app/uploaders/records_uploads.rb | 4 | ||||
-rw-r--r-- | changelogs/unreleased/migrate-artifacts-to-a-new-path.yml | 4 | ||||
-rw-r--r-- | db/post_migrate/20170523083112_migrate_old_artifacts.rb | 72 | ||||
-rw-r--r-- | lib/api/jobs.rb | 2 | ||||
-rw-r--r-- | lib/api/runner.rb | 2 | ||||
-rw-r--r-- | lib/api/v3/builds.rb | 2 | ||||
-rw-r--r-- | lib/backup/artifacts.rb | 2 | ||||
-rw-r--r-- | lib/ci/api/builds.rb | 2 | ||||
-rw-r--r-- | spec/migrations/migrate_old_artifacts_spec.rb | 117 |
14 files changed, 225 insertions, 58 deletions
diff --git a/app/controllers/projects/artifacts_controller.rb b/app/controllers/projects/artifacts_controller.rb index ea036b1f705..b400670746f 100644 --- a/app/controllers/projects/artifacts_controller.rb +++ b/app/controllers/projects/artifacts_controller.rb @@ -10,7 +10,7 @@ class Projects::ArtifactsController < Projects::ApplicationController before_action :set_path_and_entry, only: [:file, :raw] def download - if artifacts_file.file_storage? + if artifacts_file.local_file? send_file artifacts_file.path, disposition: 'attachment' else redirect_to artifacts_file.url diff --git a/app/controllers/uploads_controller.rb b/app/controllers/uploads_controller.rb index eef53730291..7493bf38bba 100644 --- a/app/controllers/uploads_controller.rb +++ b/app/controllers/uploads_controller.rb @@ -80,7 +80,7 @@ class UploadsController < ApplicationController else @uploader = @model.send(upload_mount) - redirect_to @uploader.url unless @uploader.file_storage? + redirect_to @uploader.url unless @uploader.local_storage? end @uploader diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 60b71ff0d93..63b72b8e6a9 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -255,38 +255,6 @@ module Ci Time.now - updated_at > 15.minutes.to_i end - ## - # Deprecated - # - # This contains a hotfix for CI build data integrity, see #4246 - # - # This method is used by `ArtifactUploader` to create a store_dir. - # Warning: Uploader uses it after AND before file has been stored. - # - # This method returns old path to artifacts only if it already exists. - # - def artifacts_path - # We need the project even if it's soft deleted, because whenever - # we're really deleting the project, we'll also delete the builds, - # and in order to delete the builds, we need to know where to find - # the artifacts, which is depending on the data of the project. - # We need to retain the project in this case. - the_project = project || unscoped_project - - old = File.join(created_at.utc.strftime('%Y_%m'), - the_project.ci_id.to_s, - id.to_s) - - old_store = File.join(ArtifactUploader.artifacts_path, old) - return old if the_project.ci_id && File.directory?(old_store) - - File.join( - created_at.utc.strftime('%Y_%m'), - the_project.id.to_s, - id.to_s - ) - end - def valid_token?(token) self.token && ActiveSupport::SecurityUtils.variable_size_secure_compare(token, self.token) end diff --git a/app/uploaders/artifact_uploader.rb b/app/uploaders/artifact_uploader.rb index 3e36ec91205..668b6b8e048 100644 --- a/app/uploaders/artifact_uploader.rb +++ b/app/uploaders/artifact_uploader.rb @@ -1,33 +1,31 @@ class ArtifactUploader < GitlabUploader storage :file - attr_accessor :build, :field + attr_reader :job, :field - def self.artifacts_path + def self.local_artifacts_store Gitlab.config.artifacts.path end - def self.artifacts_upload_path - File.join(self.artifacts_path, 'tmp/uploads/') + def initialize(job, field) + @job, @field = job, field end - def self.artifacts_cache_path - File.join(self.artifacts_path, 'tmp/cache/') + def store_dir + default_local_path end - def initialize(build, field) - @build, @field = build, field + def cache_dir + File.join(self.class.local_artifacts_store, 'tmp/cache') end - def store_dir - File.join(self.class.artifacts_path, @build.artifacts_path) - end + private - def cache_dir - File.join(self.class.artifacts_cache_path, @build.artifacts_path) + def default_local_path + File.join(self.class.local_artifacts_store, default_path) end - def filename - file.try(:filename) + def default_path + File.join(job.created_at.utc.strftime('%Y_%m'), job.project_id.to_s, job.id.to_s) end end diff --git a/app/uploaders/gitlab_uploader.rb b/app/uploaders/gitlab_uploader.rb index e0a6c9b4067..c1d9007dc71 100644 --- a/app/uploaders/gitlab_uploader.rb +++ b/app/uploaders/gitlab_uploader.rb @@ -9,8 +9,16 @@ class GitlabUploader < CarrierWave::Uploader::Base delegate :base_dir, to: :class - def file_storage? - self.class.storage == CarrierWave::Storage::File + def local_file? + local_storage? && file&.is_a?(CarrierWave::SanitizedFile) + end + + def local_storage? + storage.is_a?(CarrierWave::Storage::File) + end + + def local_cache_storage? + cache_storage.is_a?(CarrierWave::Storage::File) end # Reduce disk IO diff --git a/app/uploaders/records_uploads.rb b/app/uploaders/records_uploads.rb index 4c127f29250..be3a2caa60c 100644 --- a/app/uploaders/records_uploads.rb +++ b/app/uploaders/records_uploads.rb @@ -16,7 +16,7 @@ module RecordsUploads # # Called `after :store` def record_upload(_tempfile) - return unless file_storage? + return unless local_file? return unless file.exists? Upload.record(self) @@ -26,7 +26,7 @@ module RecordsUploads # # Called `before :remove` def destroy_upload(*args) - return unless file_storage? + return unless local_file? return unless file Upload.remove_path(relative_path) diff --git a/changelogs/unreleased/migrate-artifacts-to-a-new-path.yml b/changelogs/unreleased/migrate-artifacts-to-a-new-path.yml new file mode 100644 index 00000000000..bd022a3a91b --- /dev/null +++ b/changelogs/unreleased/migrate-artifacts-to-a-new-path.yml @@ -0,0 +1,4 @@ +--- +title: Migrate artifacts to a new path +merge_request: +author: diff --git a/db/post_migrate/20170523083112_migrate_old_artifacts.rb b/db/post_migrate/20170523083112_migrate_old_artifacts.rb new file mode 100644 index 00000000000..3067e3d404b --- /dev/null +++ b/db/post_migrate/20170523083112_migrate_old_artifacts.rb @@ -0,0 +1,72 @@ +class MigrateOldArtifacts < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + + # This uses special heuristic to find potential candidates for data migration + # Read more about this here: https://gitlab.com/gitlab-org/gitlab-ce/issues/32036#note_30422345 + + def up + builds_with_artifacts.find_each do |build| + build.migrate_artifacts! + end + end + + def down + end + + private + + def builds_with_artifacts + Build.with_artifacts + .joins('JOIN projects ON projects.id = ci_builds.project_id') + .where('ci_builds.id < ?', min_id || 0) + .where('projects.ci_id IS NOT NULL') + .select('id', 'created_at', 'project_id', 'projects.ci_id AS ci_id') + end + + def min_id + Build.joins('JOIN projects ON projects.id = ci_builds.project_id') + .where('projects.ci_id IS NULL') + .pluck('min(ci_builds.id)') + .first + end + + class Build < ActiveRecord::Base + self.table_name = 'ci_builds' + + scope :with_artifacts, ->() { where.not(artifacts_file: [nil, '']) } + + def migrate_artifacts! + return unless File.exists?(source_artifacts_path) + return if File.exists?(target_artifacts_path) + + ensure_target_path + + FileUtils.move(source_artifacts_path, target_artifacts_path) + end + + private + + def source_artifacts_path + @source_artifacts_path ||= + File.join(Gitlab.config.artifacts.path, + created_at.utc.strftime('%Y_%m'), + ci_id.to_s, id.to_s) + end + + def target_artifacts_path + @target_artifacts_path ||= + File.join(Gitlab.config.artifacts.path, + created_at.utc.strftime('%Y_%m'), + project_id.to_s, id.to_s) + end + + def ensure_target_path + directory = File.dirname(target_artifacts_path) + FileUtils.mkdir_p(directory) unless Dir.exist?(directory) + end + end +end diff --git a/lib/api/jobs.rb b/lib/api/jobs.rb index 0223957fde1..7f94b1afdc4 100644 --- a/lib/api/jobs.rb +++ b/lib/api/jobs.rb @@ -225,7 +225,7 @@ module API end def present_artifacts!(artifacts_file) - if !artifacts_file.file_storage? + if !artifacts_file.local_file? redirect_to(build.artifacts_file.url) elsif artifacts_file.exists? present_file!(artifacts_file.path, artifacts_file.filename) diff --git a/lib/api/runner.rb b/lib/api/runner.rb index 6fbb02cb3aa..ddcc0429de0 100644 --- a/lib/api/runner.rb +++ b/lib/api/runner.rb @@ -242,7 +242,7 @@ module API job = authenticate_job! artifacts_file = job.artifacts_file - unless artifacts_file.file_storage? + unless artifacts_file.local_file? return redirect_to job.artifacts_file.url end diff --git a/lib/api/v3/builds.rb b/lib/api/v3/builds.rb index 21935922414..7ce7b43e3bd 100644 --- a/lib/api/v3/builds.rb +++ b/lib/api/v3/builds.rb @@ -226,7 +226,7 @@ module API end def present_artifacts!(artifacts_file) - if !artifacts_file.file_storage? + if !artifacts_file.local_file? redirect_to(build.artifacts_file.url) elsif artifacts_file.exists? present_file!(artifacts_file.path, artifacts_file.filename) diff --git a/lib/backup/artifacts.rb b/lib/backup/artifacts.rb index 51fa3867e67..1f4bda6f588 100644 --- a/lib/backup/artifacts.rb +++ b/lib/backup/artifacts.rb @@ -3,7 +3,7 @@ require 'backup/files' module Backup class Artifacts < Files def initialize - super('artifacts', ArtifactUploader.artifacts_path) + super('artifacts', ArtifactUploader.local_artifacts_store) end def create_files_dir diff --git a/lib/ci/api/builds.rb b/lib/ci/api/builds.rb index 67b269b330c..0f2044a54b4 100644 --- a/lib/ci/api/builds.rb +++ b/lib/ci/api/builds.rb @@ -187,7 +187,7 @@ module Ci build = authenticate_build! artifacts_file = build.artifacts_file - unless artifacts_file.file_storage? + unless artifacts_file.local_file? return redirect_to build.artifacts_file.url end diff --git a/spec/migrations/migrate_old_artifacts_spec.rb b/spec/migrations/migrate_old_artifacts_spec.rb new file mode 100644 index 00000000000..9d951038dcc --- /dev/null +++ b/spec/migrations/migrate_old_artifacts_spec.rb @@ -0,0 +1,117 @@ +# encoding: utf-8 + +require 'spec_helper' +require Rails.root.join('db', 'post_migrate', '20170523083112_migrate_old_artifacts.rb') + +describe MigrateOldArtifacts do + let(:migration) { described_class.new } + let!(:directory) { Dir.mktmpdir } + + before do + allow(Gitlab.config.artifacts).to receive(:path).and_return(directory) + end + + after do + FileUtils.remove_entry_secure(directory) + end + + context 'with migratable data' do + let(:project1) { create(:empty_project, ci_id: 2) } + let(:project2) { create(:empty_project, ci_id: 3) } + let(:project3) { create(:empty_project) } + + let(:pipeline1) { create(:ci_empty_pipeline, project: project1) } + let(:pipeline2) { create(:ci_empty_pipeline, project: project2) } + let(:pipeline3) { create(:ci_empty_pipeline, project: project3) } + + let!(:build_with_legacy_artifacts) { create(:ci_build, pipeline: pipeline1) } + let!(:build_without_artifacts) { create(:ci_build, pipeline: pipeline1) } + let!(:build2) { create(:ci_build, :artifacts, pipeline: pipeline2) } + let!(:build3) { create(:ci_build, :artifacts, pipeline: pipeline3) } + + before do + store_artifacts_in_legacy_path(build_with_legacy_artifacts) + end + + it "legacy artifacts are not accessible" do + expect(build_with_legacy_artifacts.artifacts?).to be_falsey + end + + it "legacy artifacts are set" do + expect(build_with_legacy_artifacts.artifacts_file_identifier).not_to be_nil + end + + describe '#min_id' do + subject { migration.send(:min_id) } + + it 'returns the newest build for which ci_id is not defined' do + is_expected.to eq(build3.id) + end + end + + describe '#builds_with_artifacts' do + subject { migration.send(:builds_with_artifacts).map(&:id) } + + it 'returns a list of builds that has artifacts and could be migrated' do + is_expected.to contain_exactly(build_with_legacy_artifacts.id, build2.id) + end + end + + describe '#up' do + context 'when migrating artifacts' do + before do + migration.up + end + + it 'all files do have artifacts' do + Ci::Build.with_artifacts do |build| + expect(build).to have_artifacts + end + end + + it 'artifacts are no longer present on legacy path' do + expect(File.exist?(legacy_path(build_with_legacy_artifacts))).to eq(false) + end + end + + context 'when there are aritfacts in old and new directory' do + before do + store_artifacts_in_legacy_path(build2) + + migration.up + end + + it 'does not move old files' do + expect(File.exist?(legacy_path(build2))).to eq(true) + end + end + end + + private + + def store_artifacts_in_legacy_path(build) + FileUtils.mkdir_p(legacy_path(build)) + + FileUtils.copy( + Rails.root.join('spec/fixtures/ci_build_artifacts.zip'), + File.join(legacy_path(build), "ci_build_artifacts.zip") + ) + FileUtils.copy( + Rails.root.join('spec/fixtures/ci_build_artifacts_metadata.gz'), + File.join(legacy_path(build), "ci_build_artifacts_metadata.gz") + ) + build.update_columns( + artifacts_file: 'ci_build_artifacts.zip', + artifacts_metadata: 'ci_build_artifacts_metadata.gz', + ) + build.reload + end + + def legacy_path(build) + File.join(directory, + build.created_at.utc.strftime('%Y_%m'), + build.project.ci_id.to_s, + build.id.to_s) + end + end +end |