Commit e61f38d7 authored by Kamil Trzcinski's avatar Kamil Trzcinski

Fix data inconsistency issue for old artifacts by moving them to a currently used path

parent c72abcef
......@@ -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
......
......@@ -80,7 +80,7 @@ def uploader
else
@uploader = @model.send(upload_mount)
redirect_to @uploader.url unless @uploader.file_storage?
redirect_to @uploader.url unless @uploader.local_storage?
end
@uploader
......
......@@ -255,38 +255,6 @@ def needs_touch?
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
......
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
......@@ -9,8 +9,16 @@ def self.base_dir
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
......
......@@ -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 @@ def record_upload(_tempfile)
#
# Called `before :remove`
def destroy_upload(*args)
return unless file_storage?
return unless local_file?
return unless file
Upload.remove_path(relative_path)
......
---
title: Migrate artifacts to a new path
merge_request:
author:
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
......@@ -225,7 +225,7 @@ def get_build!(id)
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)
......
......@@ -242,7 +242,7 @@ class Runner < Grape::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
......
......@@ -226,7 +226,7 @@ def get_build!(id)
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)
......
......@@ -3,7 +3,7 @@
module Backup
class Artifacts < Files
def initialize
super('artifacts', ArtifactUploader.artifacts_path)
super('artifacts', ArtifactUploader.local_artifacts_store)
end
def create_files_dir
......
......@@ -187,7 +187,7 @@ class Builds < Grape::API
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
......
# 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
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment