From 87f11d2cf539d9539b439b54355f0dadaf4ebf76 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kamil=20Trzci=C5=84ski?= Date: Fri, 8 Dec 2017 09:09:06 +0000 Subject: [PATCH] Merge branch 'zj-auto-upload-job-artifacts' into 'master' Transfer job archives after creation See merge request gitlab-org/gitlab-ee!3646 --- app/models/ci/job_artifact.rb | 7 ++ app/models/lfs_object.rb | 9 ++ app/uploaders/lfs_object_uploader.rb | 1 - app/uploaders/object_store_uploader.rb | 11 ++- app/workers/object_storage_upload_worker.rb | 6 +- .../zj-auto-upload-job-artifacts.yml | 5 + config/gitlab.yml.example | 1 + spec/ee/spec/models/ee/lfs_object_spec.rb | 96 +++++++++++++++++++ .../object_storage_upload_worker_spec.rb | 6 +- spec/models/ci/job_artifact_spec.rb | 58 +++++++++++ spec/requests/lfs_http_spec.rb | 2 +- spec/support/stub_object_storage.rb | 3 +- spec/uploaders/lfs_object_uploader_spec.rb | 2 +- 13 files changed, 195 insertions(+), 12 deletions(-) create mode 100644 changelogs/unreleased-ee/zj-auto-upload-job-artifacts.yml create mode 100644 spec/ee/spec/models/ee/lfs_object_spec.rb rename spec/{ => ee}/workers/object_storage_upload_worker_spec.rb (93%) diff --git a/app/models/ci/job_artifact.rb b/app/models/ci/job_artifact.rb index 84fc6863567..1aea897aaca 100644 --- a/app/models/ci/job_artifact.rb +++ b/app/models/ci/job_artifact.rb @@ -1,5 +1,6 @@ module Ci class JobArtifact < ActiveRecord::Base + include AfterCommitQueue extend Gitlab::Ci::Model belongs_to :project @@ -9,6 +10,12 @@ module Ci mount_uploader :file, JobArtifactUploader + after_save if: :file_changed?, on: [:create, :update] do + run_after_commit do + file.schedule_migration_to_object_storage + end + end + enum file_type: { archive: 1, metadata: 2 diff --git a/app/models/lfs_object.rb b/app/models/lfs_object.rb index 38b8c41024a..6ad792aab30 100644 --- a/app/models/lfs_object.rb +++ b/app/models/lfs_object.rb @@ -1,4 +1,7 @@ class LfsObject < ActiveRecord::Base + prepend EE::LfsObject + include AfterCommitQueue + has_many :lfs_objects_projects, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent has_many :projects, through: :lfs_objects_projects @@ -8,6 +11,12 @@ class LfsObject < ActiveRecord::Base mount_uploader :file, LfsObjectUploader + after_save if: :file_changed?, on: [:create, :update] do + run_after_commit do + file.schedule_migration_to_object_storage + end + end + def project_allowed_access?(project) projects.exists?(project.lfs_storage_project.id) end diff --git a/app/uploaders/lfs_object_uploader.rb b/app/uploaders/lfs_object_uploader.rb index 88cf0450dcd..fa42e4710b7 100644 --- a/app/uploaders/lfs_object_uploader.rb +++ b/app/uploaders/lfs_object_uploader.rb @@ -1,6 +1,5 @@ class LfsObjectUploader < ObjectStoreUploader storage_options Gitlab.config.lfs - after :store, :schedule_migration_to_object_storage def self.local_store_path Gitlab.config.lfs.storage_path diff --git a/app/uploaders/object_store_uploader.rb b/app/uploaders/object_store_uploader.rb index b5de0357a5f..bb25dc4219f 100644 --- a/app/uploaders/object_store_uploader.rb +++ b/app/uploaders/object_store_uploader.rb @@ -116,10 +116,13 @@ class ObjectStoreUploader < GitlabUploader end end - def schedule_migration_to_object_storage(new_file) - if self.class.object_store_enabled? && licensed? && file_storage? - ObjectStorageUploadWorker.perform_async(self.class.name, model.class.name, mounted_as, model.id) - end + def schedule_migration_to_object_storage(*args) + return unless self.class.object_store_enabled? + return unless self.class.background_upload_enabled? + return unless self.licensed? + return unless self.file_storage? + + ObjectStorageUploadWorker.perform_async(self.class.name, model.class.name, mounted_as, model.id) end def fog_directory diff --git a/app/workers/object_storage_upload_worker.rb b/app/workers/object_storage_upload_worker.rb index 0a374c4323f..0b9411ff2df 100644 --- a/app/workers/object_storage_upload_worker.rb +++ b/app/workers/object_storage_upload_worker.rb @@ -2,6 +2,8 @@ class ObjectStorageUploadWorker include Sidekiq::Worker include DedicatedSidekiqQueue + sidekiq_options retry: 5 + def perform(uploader_class_name, subject_class_name, file_field, subject_id) uploader_class = uploader_class_name.constantize subject_class = subject_class_name.constantize @@ -9,7 +11,9 @@ class ObjectStorageUploadWorker 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/changelogs/unreleased-ee/zj-auto-upload-job-artifacts.yml b/changelogs/unreleased-ee/zj-auto-upload-job-artifacts.yml new file mode 100644 index 00000000000..4335f85e360 --- /dev/null +++ b/changelogs/unreleased-ee/zj-auto-upload-job-artifacts.yml @@ -0,0 +1,5 @@ +--- +title: Transfer job archives to object storage after creation +merge_request: +author: +type: added diff --git a/config/gitlab.yml.example b/config/gitlab.yml.example index e2256c5c118..d8fa3138184 100644 --- a/config/gitlab.yml.example +++ b/config/gitlab.yml.example @@ -679,6 +679,7 @@ test: object_store: enabled: false remote_directory: artifacts # The bucket name + background_upload: false connection: provider: AWS # Only AWS supported at the moment aws_access_key_id: AWS_ACCESS_KEY_ID diff --git a/spec/ee/spec/models/ee/lfs_object_spec.rb b/spec/ee/spec/models/ee/lfs_object_spec.rb new file mode 100644 index 00000000000..b02327b4c73 --- /dev/null +++ b/spec/ee/spec/models/ee/lfs_object_spec.rb @@ -0,0 +1,96 @@ +require 'spec_helper' + +describe LfsObject do + describe '#local_store?' do + it 'returns true when file_store is nil' do + subject.file_store = nil + + expect(subject.local_store?).to eq true + end + + it 'returns true when file_store is equal to LfsObjectUploader::LOCAL_STORE' do + subject.file_store = LfsObjectUploader::LOCAL_STORE + + expect(subject.local_store?).to eq true + end + + it 'returns false whe file_store is equal to LfsObjectUploader::REMOTE_STORE' do + subject.file_store = LfsObjectUploader::REMOTE_STORE + + expect(subject.local_store?).to eq false + end + end + + describe '#destroy' do + subject { create(:lfs_object, :with_file) } + + context 'when running in a Geo primary node' do + set(:primary) { create(:geo_node, :primary) } + set(:secondary) { create(:geo_node) } + + it 'logs an event to the Geo event log' do + expect { subject.destroy }.to change(Geo::LfsObjectDeletedEvent, :count).by(1) + end + end + end + + describe '#schedule_migration_to_object_storage' do + before do + stub_lfs_setting(enabled: true) + end + + subject { create(:lfs_object, :with_file) } + + context 'when object storage is disabled' do + before do + stub_lfs_object_storage(enabled: false) + end + + it 'does not schedule the migration' do + expect(ObjectStorageUploadWorker).not_to receive(:perform_async) + + subject + end + end + + context 'when object storage is enabled' do + context 'when background upload is enabled' do + context 'when is licensed' do + before do + stub_lfs_object_storage(background_upload: true) + end + + it 'schedules the model for migration' do + expect(ObjectStorageUploadWorker).to receive(:perform_async).with('LfsObjectUploader', described_class.name, :file, kind_of(Numeric)) + + subject + end + end + + context 'when is unlicensed' do + before do + stub_lfs_object_storage(background_upload: true, licensed: false) + end + + it 'does not schedule the migration' do + expect(ObjectStorageUploadWorker).not_to receive(:perform_async) + + subject + end + end + end + + context 'when background upload is disabled' do + before do + stub_lfs_object_storage(background_upload: false) + end + + it 'schedules the model for migration' do + expect(ObjectStorageUploadWorker).not_to receive(:perform_async) + + subject + end + end + end + end +end diff --git a/spec/workers/object_storage_upload_worker_spec.rb b/spec/ee/workers/object_storage_upload_worker_spec.rb similarity index 93% rename from spec/workers/object_storage_upload_worker_spec.rb rename to spec/ee/workers/object_storage_upload_worker_spec.rb index 0922b5feccd..d421fdf95a9 100644 --- a/spec/workers/object_storage_upload_worker_spec.rb +++ b/spec/ee/workers/object_storage_upload_worker_spec.rb @@ -17,7 +17,7 @@ describe ObjectStorageUploadWorker do context 'when object storage is enabled' do before do - stub_lfs_object_storage + stub_lfs_object_storage(background_upload: true) end it 'uploads object to storage' do @@ -60,7 +60,7 @@ describe ObjectStorageUploadWorker do context 'and remote storage is defined' do before do - stub_artifacts_object_storage + stub_artifacts_object_storage(background_upload: true) end it "migrates file to remote storage" do @@ -94,7 +94,7 @@ describe ObjectStorageUploadWorker do context 'and remote storage is defined' do before do - stub_artifacts_object_storage + stub_artifacts_object_storage(background_upload: true) end it "migrates file to remote storage" do diff --git a/spec/models/ci/job_artifact_spec.rb b/spec/models/ci/job_artifact_spec.rb index 0e18a326c68..a10afb98d2b 100644 --- a/spec/models/ci/job_artifact_spec.rb +++ b/spec/models/ci/job_artifact_spec.rb @@ -12,6 +12,64 @@ describe Ci::JobArtifact do it { is_expected.to respond_to(:created_at) } it { is_expected.to respond_to(:updated_at) } + describe 'callbacks' do + subject { create(:ci_job_artifact, :archive) } + + describe '#schedule_migration_to_object_storage' do + context 'when object storage is disabled' do + before do + stub_artifacts_object_storage(enabled: false) + end + + it 'does not schedule the migration' do + expect(ObjectStorageUploadWorker).not_to receive(:perform_async) + + subject + end + end + + context 'when object storage is enabled' do + context 'when background upload is enabled' do + context 'when is licensed' do + before do + stub_artifacts_object_storage(background_upload: true) + end + + it 'schedules the model for migration' do + expect(ObjectStorageUploadWorker).to receive(:perform_async).with('JobArtifactUploader', described_class.name, :file, kind_of(Numeric)) + + subject + end + end + + context 'when is unlicensed' do + before do + stub_artifacts_object_storage(background_upload: true, licensed: false) + end + + it 'does not schedule the migration' do + expect(ObjectStorageUploadWorker).not_to receive(:perform_async) + + subject + end + end + end + + context 'when background upload is disabled' do + before do + stub_artifacts_object_storage(background_upload: false) + end + + it 'schedules the model for migration' do + expect(ObjectStorageUploadWorker).not_to receive(:perform_async) + + subject + end + end + end + end + end + describe '#set_size' do it 'sets the size' do expect(artifact.size).to eq(106365) diff --git a/spec/requests/lfs_http_spec.rb b/spec/requests/lfs_http_spec.rb index b5948505701..d7bdfde918c 100644 --- a/spec/requests/lfs_http_spec.rb +++ b/spec/requests/lfs_http_spec.rb @@ -1010,7 +1010,7 @@ describe 'Git LFS API and storage' do context 'with object storage enabled' do before do - stub_lfs_object_storage + stub_lfs_object_storage(background_upload: true) end it 'schedules migration of file to object storage' do diff --git a/spec/support/stub_object_storage.rb b/spec/support/stub_object_storage.rb index cf5746bc29f..4f469648d5c 100644 --- a/spec/support/stub_object_storage.rb +++ b/spec/support/stub_object_storage.rb @@ -1,8 +1,9 @@ module StubConfiguration - def stub_object_storage_uploader(config:, uploader:, remote_directory:, enabled: true, licensed: true) + def stub_object_storage_uploader(config:, uploader:, remote_directory:, enabled: true, licensed: true, background_upload: false) Fog.mock! allow(config).to receive(:enabled) { enabled } + allow(config).to receive(:background_upload) { background_upload } stub_licensed_features(object_storage: licensed) unless licensed == :skip diff --git a/spec/uploaders/lfs_object_uploader_spec.rb b/spec/uploaders/lfs_object_uploader_spec.rb index 1e09958d369..9b8e2835ebc 100644 --- a/spec/uploaders/lfs_object_uploader_spec.rb +++ b/spec/uploaders/lfs_object_uploader_spec.rb @@ -49,7 +49,7 @@ describe LfsObjectUploader do context 'with object storage enabled' do before do - stub_lfs_object_storage + stub_lfs_object_storage(background_upload: true) end it 'is scheduled to run after creation' do