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

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

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

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

app/uploaders/object_store_uploader.rb

@@ -116,11 +116,14 @@ class ObjectStoreUploader < GitlabUploader
     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_options.remote_directory

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?

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

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

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

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

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

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
 

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