Merge branch 'mc/feature/keep-latest-artifact-for-ref' into 'master'

Keep latest artifact for ref

See merge request gitlab-org/gitlab!29802

app/models/ci/job_artifact.rb

@@ -70,6 +70,10 @@ module Ci
 
     TYPE_AND_FORMAT_PAIRS = INTERNAL_TYPES.merge(REPORT_TYPES).freeze
 
+    # This is required since we cannot add a default to the database
+    # https://gitlab.com/gitlab-org/gitlab/-/issues/215418
+    attribute :locked, :boolean, default: false
+
     belongs_to :project
     belongs_to :job, class_name: "Ci::Build", foreign_key: :job_id
 
@@ -86,6 +90,7 @@ module Ci
     scope :with_files_stored_locally, -> { where(file_store: [nil, ::JobArtifactUploader::Store::LOCAL]) }
     scope :with_files_stored_remotely, -> { where(file_store: ::JobArtifactUploader::Store::REMOTE) }
     scope :for_sha, ->(sha, project_id) { joins(job: :pipeline).where(ci_pipelines: { sha: sha, project_id: project_id }) }
+    scope :for_ref, ->(ref, project_id) { joins(job: :pipeline).where(ci_pipelines: { ref: ref, project_id: project_id }) }
     scope :for_job_name, ->(name) { joins(:job).where(ci_builds: { name: name }) }
 
     scope :with_file_types, -> (file_types) do
@@ -121,6 +126,8 @@ module Ci
     end
 
     scope :expired, -> (limit) { where('expire_at < ?', Time.now).limit(limit) }
+    scope :locked, -> { where(locked: true) }
+    scope :unlocked, -> { where(locked: [false, nil]) }
 
     scope :scoped_project, -> { where('ci_job_artifacts.project_id = projects.id') }
 

app/services/ci/create_job_artifacts_service.rb

@@ -33,7 +33,8 @@ module Ci
         file_type: params['artifact_type'],
         file_format: params['artifact_format'],
         file_sha256: artifacts_file.sha256,
-        expire_in: expire_in)
+        expire_in: expire_in,
+        locked: true)
 
       artifact_metadata = if metadata_file
                             Ci::JobArtifact.new(
@@ -43,7 +44,8 @@ module Ci
                               file_type: :metadata,
                               file_format: :gzip,
                               file_sha256: metadata_file.sha256,
-                              expire_in: expire_in)
+                              expire_in: expire_in,
+                              locked: true)
                           end
 
       [artifact, artifact_metadata]
@@ -64,6 +66,7 @@ module Ci
       Ci::JobArtifact.transaction do
         artifact.save!
         artifact_metadata&.save!
+        unlock_previous_artifacts!(artifact)
 
         # NOTE: The `artifacts_expire_at` column is already deprecated and to be removed in the near future.
         job.update_column(:artifacts_expire_at, artifact.expire_at)
@@ -81,6 +84,10 @@ module Ci
       error(error.message, :bad_request)
     end
 
+    def unlock_previous_artifacts!(artifact)
+      Ci::JobArtifact.for_ref(artifact.job.ref, artifact.project_id).locked.update_all(locked: false)
+    end
+
     def sha256_matches_existing_artifact?(job, artifact_type, artifacts_file)
       existing_artifact = job.job_artifacts.find_by_file_type(artifact_type)
       return false unless existing_artifact

app/services/ci/destroy_expired_job_artifacts_service.rb

@@ -28,7 +28,7 @@ module Ci
     private
 
     def destroy_batch
-      artifacts = Ci::JobArtifact.expired(BATCH_SIZE).to_a
+      artifacts = Ci::JobArtifact.expired(BATCH_SIZE).unlocked.to_a
 
       return false if artifacts.empty?
 

changelogs/unreleased/mc-feature-keep-latest-artifact-for-ref.yml

+---
+title: Keep latest artifact for each ref.
+merge_request: 29802
+author:
+type: changed

db/migrate/20200417145946_add_locked_to_ci_job_artifact.rb

+# frozen_string_literal: true
+
+class AddLockedToCiJobArtifact < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def up
+    with_lock_retries do
+      add_column :ci_job_artifacts, :locked, :boolean
+    end
+  end
+
+  def down
+    with_lock_retries do
+      remove_column :ci_job_artifacts, :locked
+    end
+  end
+end

db/structure.sql

@@ -1079,7 +1079,8 @@ CREATE TABLE public.ci_job_artifacts (
     file_store integer,
     file_sha256 bytea,
     file_format smallint,
-    file_location smallint
+    file_location smallint,
+    locked boolean
 );
 
 CREATE SEQUENCE public.ci_job_artifacts_id_seq
@@ -13509,6 +13510,7 @@ COPY "schema_migrations" (version) FROM STDIN;
 20200416120128
 20200416120354
 20200417044453
+20200417145946
 20200420104303
 20200420104323
 20200420162730

doc/ci/yaml/README.md

@@ -2939,6 +2939,10 @@ job:
     expire_in: 1 week
 ```
 
+NOTE: **Note:**
+For artifacts created in [GitLab 13.0](https://gitlab.com/gitlab-org/gitlab/-/issues/16267)
+and later, the latest artifact for a ref is always kept, regardless of the expiry time.
+
 #### `artifacts:reports`
 
 > [Introduced](https://gitlab.com/gitlab-org/gitlab-foss/-/merge_requests/20390) in GitLab 11.2. Requires GitLab Runner 11.2 and above.

spec/models/ci/job_artifact_spec.rb

@@ -160,15 +160,26 @@ describe Ci::JobArtifact do
   end
 
   describe '.for_sha' do
+    let(:first_pipeline) { create(:ci_pipeline) }
+    let(:second_pipeline) { create(:ci_pipeline, project: first_pipeline.project, sha: Digest::SHA1.hexdigest(SecureRandom.hex)) }
+    let!(:first_artifact) { create(:ci_job_artifact, job: create(:ci_build, pipeline: first_pipeline)) }
+    let!(:second_artifact) { create(:ci_job_artifact, job: create(:ci_build, pipeline: second_pipeline)) }
+
     it 'returns job artifacts for a given pipeline sha' do
-      project = create(:project)
-      first_pipeline = create(:ci_pipeline, project: project)
-      second_pipeline = create(:ci_pipeline, project: project, sha: Digest::SHA1.hexdigest(SecureRandom.hex))
-      first_artifact = create(:ci_job_artifact, job: create(:ci_build, pipeline: first_pipeline))
-      second_artifact = create(:ci_job_artifact, job: create(:ci_build, pipeline: second_pipeline))
+      expect(described_class.for_sha(first_pipeline.sha, first_pipeline.project.id)).to eq([first_artifact])
+      expect(described_class.for_sha(second_pipeline.sha, first_pipeline.project.id)).to eq([second_artifact])
+    end
+  end
+
+  describe '.for_ref' do
+    let(:first_pipeline) { create(:ci_pipeline, ref: 'first_ref') }
+    let(:second_pipeline) { create(:ci_pipeline, ref: 'second_ref', project: first_pipeline.project) }
+    let!(:first_artifact) { create(:ci_job_artifact, job: create(:ci_build, pipeline: first_pipeline)) }
+    let!(:second_artifact) { create(:ci_job_artifact, job: create(:ci_build, pipeline: second_pipeline)) }
 
-      expect(described_class.for_sha(first_pipeline.sha, project.id)).to eq([first_artifact])
-      expect(described_class.for_sha(second_pipeline.sha, project.id)).to eq([second_artifact])
+    it 'returns job artifacts for a given pipeline ref' do
+      expect(described_class.for_ref(first_pipeline.ref, first_pipeline.project.id)).to eq([first_artifact])
+      expect(described_class.for_ref(second_pipeline.ref, first_pipeline.project.id)).to eq([second_artifact])
     end
   end
 
@@ -185,9 +196,9 @@ describe Ci::JobArtifact do
   end
 
   describe 'callbacks' do
-    subject { create(:ci_job_artifact, :archive) }
-
     describe '#schedule_background_upload' do
+      subject { create(:ci_job_artifact, :archive) }
+
       context 'when object storage is disabled' do
         before do
           stub_artifacts_object_storage(enabled: false)

spec/services/ci/create_job_artifacts_service_spec.rb

@@ -30,6 +30,26 @@ describe Ci::CreateJobArtifactsService do
   describe '#execute' do
     subject { service.execute(job, artifacts_file, params, metadata_file: metadata_file) }
 
+    context 'locking' do
+      let(:old_job) { create(:ci_build, pipeline: create(:ci_pipeline, project: job.project, ref: job.ref)) }
+      let!(:latest_artifact) { create(:ci_job_artifact, job: old_job, locked: true) }
+      let!(:other_artifact) { create(:ci_job_artifact, locked: true) }
+
+      it 'locks the new artifact' do
+        subject
+
+        expect(Ci::JobArtifact.last).to have_attributes(locked: true)
+      end
+
+      it 'unlocks all other artifacts for the same ref' do
+        expect { subject }.to change { latest_artifact.reload.locked }.from(true).to(false)
+      end
+
+      it 'does not unlock artifacts for other refs' do
+        expect { subject }.not_to change { other_artifact.reload.locked }.from(true)
+      end
+    end
+
     context 'when artifacts file is uploaded' do
       it 'saves artifact for the given type' do
         expect { subject }.to change { Ci::JobArtifact.count }.by(1)

spec/services/ci/destroy_expired_job_artifacts_service_spec.rb

@@ -11,8 +11,26 @@ describe Ci::DestroyExpiredJobArtifactsService, :clean_gitlab_redis_shared_state
     let(:service) { described_class.new }
     let!(:artifact) { create(:ci_job_artifact, expire_at: 1.day.ago) }
 
-    it 'destroys expired job artifacts' do
-      expect { subject }.to change { Ci::JobArtifact.count }.by(-1)
+    context 'when artifact is expired' do
+      context 'when artifact is not locked' do
+        before do
+          artifact.update!(locked: false)
+        end
+
+        it 'destroys job artifact' do
+          expect { subject }.to change { Ci::JobArtifact.count }.by(-1)
+        end
+      end
+
+      context 'when artifact is locked' do
+        before do
+          artifact.update!(locked: true)
+        end
+
+        it 'does not destroy job artifact' do
+          expect { subject }.not_to change { Ci::JobArtifact.count }
+        end
+      end
     end
 
     context 'when artifact is not expired' do
@@ -72,7 +90,7 @@ describe Ci::DestroyExpiredJobArtifactsService, :clean_gitlab_redis_shared_state
         stub_const('Ci::DestroyExpiredJobArtifactsService::BATCH_SIZE', 1)
       end
 
-      let!(:artifact) { create_list(:ci_job_artifact, 2, expire_at: 1.day.ago) }
+      let!(:second_artifact) { create(:ci_job_artifact, expire_at: 1.day.ago) }
 
       it 'raises an error and does not continue destroying' do
         is_expected.to be_falsy
@@ -96,7 +114,7 @@ describe Ci::DestroyExpiredJobArtifactsService, :clean_gitlab_redis_shared_state
         stub_const('Ci::DestroyExpiredJobArtifactsService::BATCH_SIZE', 1)
       end
 
-      let!(:artifact) { create_list(:ci_job_artifact, 2, expire_at: 1.day.ago) }
+      let!(:second_artifact) { create(:ci_job_artifact, expire_at: 1.day.ago) }
 
       it 'destroys all expired artifacts' do
         expect { subject }.to change { Ci::JobArtifact.count }.by(-2)