Commit b20d710a authored by Matija Čupić's avatar Matija Čupić Committed by Mayra Cabrera

Delete only unlocked expired artifacts

Delete only expired artifacts that are also unlocked (i.e. they're not
the latest for the ref).
parent cc08fb76
......@@ -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') }
......
......@@ -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
......
......@@ -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?
......
---
title: Keep latest artifact for each ref.
merge_request: 29802
author:
type: changed
# 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
......@@ -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
......@@ -13471,6 +13472,7 @@ COPY "schema_migrations" (version) FROM STDIN;
20200416120128
20200416120354
20200417044453
20200417145946
20200420104303
20200420104323
20200420162730
......
......@@ -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.
......
......@@ -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)
......
......@@ -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)
......
......@@ -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)
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment