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

 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

app/models/lfs_object.rb

 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
@@ -10,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

app/uploaders/lfs_object_uploader.rb

 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

changelogs/unreleased-ee/zj-auto-upload-job-artifacts.yml

+---
+title: Transfer job archives to object storage after creation
+merge_request:
+author:
+type: added

config/gitlab.yml.example

@@ -771,6 +771,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

ee/app/uploaders/object_store_uploader.rb

@@ -116,11 +116,14 @@ class ObjectStoreUploader < CarrierWave::Uploader::Base
     end
   end
 
-  def schedule_migration_to_object_storage(new_file)
-    if self.class.object_store_enabled? && licensed? && file_storage?
+  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
-  end
 
   def fog_directory
     self.class.object_store_directory

ee/app/workers/object_storage_upload_worker.rb

 class ObjectStorageUploadWorker
   include ApplicationWorker
 
+  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
@@ -8,7 +10,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?

spec/ee/spec/models/ee/lfs_object_spec.rb

@@ -33,4 +33,64 @@ describe LfsObject do
       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

spec/workers/object_storage_upload_worker_spec.rb → 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

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)

spec/requests/lfs_http_spec.rb

@@ -1038,7 +1038,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

spec/support/stub_object_storage.rb

 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
 

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