From 587d5d8339e9d0263c12b2b555bed3fb1862a16d Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Thu, 14 Sep 2017 11:16:12 +0200 Subject: [PATCH] Auto migrate artifacts to object storage Only triggered when the job is actually done. Also fixes an issue where the object might be deleted by another action or process and not existing anymore. --- app/models/commit_status.rb | 4 +++ app/uploaders/object_store_uploader.rb | 3 +- app/workers/object_storage_upload_worker.rb | 4 ++- doc/administration/job_artifacts.md | 2 +- ee/app/models/ee/build.rb | 10 ++++++ spec/uploaders/artifact_uploader_spec.rb | 35 ++++++++++++++++++++ spec/uploaders/object_store_uploader_spec.rb | 2 ++ 7 files changed, 57 insertions(+), 3 deletions(-) diff --git a/app/models/commit_status.rb b/app/models/commit_status.rb index f38885289404eb..506d93fe5687fe 100644 --- a/app/models/commit_status.rb +++ b/app/models/commit_status.rb @@ -159,6 +159,10 @@ def has_trace? false end + def finished? + !finished_at.nil? + end + def auto_canceled? canceled? && auto_canceled_by_id? end diff --git a/app/uploaders/object_store_uploader.rb b/app/uploaders/object_store_uploader.rb index c39f23c0fc6dbc..dc962d423df04c 100644 --- a/app/uploaders/object_store_uploader.rb +++ b/app/uploaders/object_store_uploader.rb @@ -118,12 +118,13 @@ def migrate!(new_store) raise e end + old_file.delete end end end - def schedule_migration_to_object_storage(new_file) + def schedule_migration_to_object_storage(*) if self.class.object_store_enabled? && licensed? && file_storage? ObjectStorageUploadWorker.perform_async(self.class.name, subject.class.name, field, subject.id) end diff --git a/app/workers/object_storage_upload_worker.rb b/app/workers/object_storage_upload_worker.rb index 0a374c4323f104..ff9928926125d7 100644 --- a/app/workers/object_storage_upload_worker.rb +++ b/app/workers/object_storage_upload_worker.rb @@ -9,7 +9,9 @@ def perform(uploader_class_name, subject_class_name, file_field, subject_id) return unless uploader_class.object_store_enabled? return unless uploader_class.background_upload_enabled? - subject = subject_class.find(subject_id) + subject = subject_class.find_by(id: subject_id) + return unless subject + file = subject.public_send(file_field) # rubocop:disable GitlabSecurity/PublicSend return unless file.licensed? diff --git a/doc/administration/job_artifacts.md b/doc/administration/job_artifacts.md index 3382b682b1dc88..d1436e7d25da3a 100644 --- a/doc/administration/job_artifacts.md +++ b/doc/administration/job_artifacts.md @@ -89,7 +89,7 @@ _The artifacts are stored by default in >**Notes:** - [Introduced][ee-1762] in [GitLab Enterprise Edition Premium][eep] 9.4. -- Since version 9.5, artifacts are [browsable], when object storage is enabled. +- Since version 9.5, artifacts are [browsable], when object storage is enabled. 9.4 lacks this feature. If you don't want to use the local disk where GitLab is installed to store the diff --git a/ee/app/models/ee/build.rb b/ee/app/models/ee/build.rb index 20ed6a0a00e29d..2a3dcd10bc4b74 100644 --- a/ee/app/models/ee/build.rb +++ b/ee/app/models/ee/build.rb @@ -8,6 +8,16 @@ module Build included do after_save :stick_build_if_status_changed + after_commit :schedule_migration_to_object_storage + end + + # Doing it after the commit circumvents the scheduling of jobs for sidekiq + # in a transaction + def schedule_migration_to_object_storage + if finished? + artifacts_file.schedule_migration_to_object_storage + artifacts_metadata.schedule_migration_to_object_storage + end end def shared_runners_minutes_limit_enabled? diff --git a/spec/uploaders/artifact_uploader_spec.rb b/spec/uploaders/artifact_uploader_spec.rb index 6e6864cc65db93..312be29a721fd9 100644 --- a/spec/uploaders/artifact_uploader_spec.rb +++ b/spec/uploaders/artifact_uploader_spec.rb @@ -58,6 +58,41 @@ it { is_expected.to end_with('/tmp/work') } end + describe 'migration to object storage' do + context 'with object storage disabled' do + it "is skipped" do + expect(ObjectStorageUploadWorker).not_to receive(:perform_async) + + job + end + end + + context 'with object storage enabled' do + before do + stub_artifacts_object_storage + end + + it 'is scheduled to run after creation' do + expect(ObjectStorageUploadWorker).to receive(:perform_async).with(described_class.name, 'Ci::Build', :artifacts_file, kind_of(Numeric)) + expect(ObjectStorageUploadWorker).to receive(:perform_async).with(described_class.name, 'Ci::Build', :artifacts_metadata, kind_of(Numeric)) + + job + end + end + + context 'with object storage unlicenced' do + before do + stub_artifacts_object_storage(licensed: false) + end + + it 'is skipped' do + expect(ObjectStorageUploadWorker).not_to receive(:perform_async) + + job + end + end + end + describe '#filename' do # we need to use uploader, as this makes to use mounter # which initialises uploader.file object diff --git a/spec/uploaders/object_store_uploader_spec.rb b/spec/uploaders/object_store_uploader_spec.rb index 308e068ff47cff..efb24c95dc974f 100644 --- a/spec/uploaders/object_store_uploader_spec.rb +++ b/spec/uploaders/object_store_uploader_spec.rb @@ -152,6 +152,8 @@ before do stub_artifacts_object_storage + allow(Gitlab.config.artifacts.object_store) + .to receive(:background_upload).and_return(false) end it "local file does not exist" do -- GitLab