From af9efed71e5739b63bf78c1d08dcc5399ac388dc Mon Sep 17 00:00:00 2001 From: Steve Abrams Date: Fri, 5 Nov 2021 08:21:47 -0600 Subject: [PATCH] Add read_at to dependency proxy objects Add a read_at column to dependency_proxy_manifests and dependency_proxy_blobs to be used for more accurate TTL policy logic. Update the Dependency Proxy TTL worker to use the new read_at column when qualifying objects for expiration. Changelog: changed --- ...endency_proxy_for_containers_controller.rb | 2 -- app/models/concerns/ttl_expirable.rb | 7 ++++- .../find_or_create_blob_service.rb | 3 +-- .../find_or_create_manifest_service.rb | 2 ++ .../image_ttl_group_policy_worker.rb | 5 ++-- ...d_read_at_to_dependency_proxy_manifests.rb | 7 +++++ ...3_add_read_at_to_dependency_proxy_blobs.rb | 7 +++++ ...e_dependency_proxy_indexes_with_read_at.rb | 27 +++++++++++++++++++ db/schema_migrations/20211105125756 | 1 + db/schema_migrations/20211105125813 | 1 + db/schema_migrations/20211108203248 | 1 + db/structure.sql | 8 +++--- .../find_or_create_blob_service_spec.rb | 2 +- .../find_or_create_manifest_service_spec.rb | 2 +- .../concerns/ttl_expirable_shared_examples.rb | 15 ++++++++--- .../image_ttl_group_policy_worker_spec.rb | 4 +-- 16 files changed, 76 insertions(+), 18 deletions(-) create mode 100644 db/migrate/20211105125756_add_read_at_to_dependency_proxy_manifests.rb create mode 100644 db/migrate/20211105125813_add_read_at_to_dependency_proxy_blobs.rb create mode 100644 db/migrate/20211108203248_update_dependency_proxy_indexes_with_read_at.rb create mode 100644 db/schema_migrations/20211105125756 create mode 100644 db/schema_migrations/20211105125813 create mode 100644 db/schema_migrations/20211108203248 diff --git a/app/controllers/groups/dependency_proxy_for_containers_controller.rb b/app/controllers/groups/dependency_proxy_for_containers_controller.rb index 8707381bb681..fc930ffebbda 100644 --- a/app/controllers/groups/dependency_proxy_for_containers_controller.rb +++ b/app/controllers/groups/dependency_proxy_for_containers_controller.rb @@ -113,8 +113,6 @@ def blob_via_workhorse end def send_manifest(manifest, from_cache:) - # Technical debt: change to read_at https://gitlab.com/gitlab-org/gitlab/-/issues/341536 - manifest.touch response.headers[DependencyProxy::Manifest::DIGEST_HEADER] = manifest.digest response.headers['Content-Length'] = manifest.size response.headers['Docker-Distribution-Api-Version'] = DependencyProxy::DISTRIBUTION_API_VERSION diff --git a/app/models/concerns/ttl_expirable.rb b/app/models/concerns/ttl_expirable.rb index 00abe0a06e62..6d89521255ce 100644 --- a/app/models/concerns/ttl_expirable.rb +++ b/app/models/concerns/ttl_expirable.rb @@ -5,10 +5,11 @@ module TtlExpirable included do validates :status, presence: true + default_value_for :read_at, Time.zone.now enum status: { default: 0, expired: 1, processing: 2, error: 3 } - scope :updated_before, ->(number_of_days) { where("updated_at <= ?", Time.zone.now - number_of_days.days) } + scope :read_before, ->(number_of_days) { where("read_at <= ?", Time.zone.now - number_of_days.days) } scope :active, -> { where(status: :default) } scope :lock_next_by, ->(sort) do @@ -17,4 +18,8 @@ module TtlExpirable .lock('FOR UPDATE SKIP LOCKED') end end + + def read! + self.update(read_at: Time.zone.now) + end end diff --git a/app/services/dependency_proxy/find_or_create_blob_service.rb b/app/services/dependency_proxy/find_or_create_blob_service.rb index 0a6db6e3d349..1b43263a3ba0 100644 --- a/app/services/dependency_proxy/find_or_create_blob_service.rb +++ b/app/services/dependency_proxy/find_or_create_blob_service.rb @@ -30,8 +30,7 @@ def execute blob.save! end - # Technical debt: change to read_at https://gitlab.com/gitlab-org/gitlab/-/issues/341536 - blob.touch if from_cache + blob.read! if from_cache success(blob: blob, from_cache: from_cache) end diff --git a/app/services/dependency_proxy/find_or_create_manifest_service.rb b/app/services/dependency_proxy/find_or_create_manifest_service.rb index 055980c431f8..aeb62be9f3a3 100644 --- a/app/services/dependency_proxy/find_or_create_manifest_service.rb +++ b/app/services/dependency_proxy/find_or_create_manifest_service.rb @@ -58,6 +58,8 @@ def cached_manifest_matches?(head_result) def respond(from_cache: true) if @manifest + @manifest.read! + success(manifest: @manifest, from_cache: from_cache) else error('Failed to download the manifest from the external registry', 503) diff --git a/app/workers/dependency_proxy/image_ttl_group_policy_worker.rb b/app/workers/dependency_proxy/image_ttl_group_policy_worker.rb index fed469e6dc88..6a1de00ce805 100644 --- a/app/workers/dependency_proxy/image_ttl_group_policy_worker.rb +++ b/app/workers/dependency_proxy/image_ttl_group_policy_worker.rb @@ -13,9 +13,8 @@ class ImageTtlGroupPolicyWorker # rubocop:disable Scalability/IdempotentWorker def perform DependencyProxy::ImageTtlGroupPolicy.enabled.each do |policy| - # Technical Debt: change to read_before https://gitlab.com/gitlab-org/gitlab/-/issues/341536 - qualified_blobs = policy.group.dependency_proxy_blobs.active.updated_before(policy.ttl) - qualified_manifests = policy.group.dependency_proxy_manifests.active.updated_before(policy.ttl) + qualified_blobs = policy.group.dependency_proxy_blobs.active.read_before(policy.ttl) + qualified_manifests = policy.group.dependency_proxy_manifests.active.read_before(policy.ttl) enqueue_blob_cleanup_job if expire_artifacts(qualified_blobs, DependencyProxy::Blob) enqueue_manifest_cleanup_job if expire_artifacts(qualified_manifests, DependencyProxy::Manifest) diff --git a/db/migrate/20211105125756_add_read_at_to_dependency_proxy_manifests.rb b/db/migrate/20211105125756_add_read_at_to_dependency_proxy_manifests.rb new file mode 100644 index 000000000000..a594674f470b --- /dev/null +++ b/db/migrate/20211105125756_add_read_at_to_dependency_proxy_manifests.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +class AddReadAtToDependencyProxyManifests < Gitlab::Database::Migration[1.0] + def change + add_column :dependency_proxy_manifests, :read_at, :datetime_with_timezone, null: false, default: -> { 'NOW()' } + end +end diff --git a/db/migrate/20211105125813_add_read_at_to_dependency_proxy_blobs.rb b/db/migrate/20211105125813_add_read_at_to_dependency_proxy_blobs.rb new file mode 100644 index 000000000000..1808a5414989 --- /dev/null +++ b/db/migrate/20211105125813_add_read_at_to_dependency_proxy_blobs.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +class AddReadAtToDependencyProxyBlobs < Gitlab::Database::Migration[1.0] + def change + add_column :dependency_proxy_blobs, :read_at, :datetime_with_timezone, null: false, default: -> { 'NOW()' } + end +end diff --git a/db/migrate/20211108203248_update_dependency_proxy_indexes_with_read_at.rb b/db/migrate/20211108203248_update_dependency_proxy_indexes_with_read_at.rb new file mode 100644 index 000000000000..ac0f0ddca173 --- /dev/null +++ b/db/migrate/20211108203248_update_dependency_proxy_indexes_with_read_at.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true + +class UpdateDependencyProxyIndexesWithReadAt < Gitlab::Database::Migration[1.0] + disable_ddl_transaction! + + NEW_BLOB_INDEX = 'index_dependency_proxy_blobs_on_group_id_status_read_at_id' + OLD_BLOB_INDEX = 'index_dependency_proxy_blobs_on_group_id_status_and_id' + + NEW_MANIFEST_INDEX = 'index_dependency_proxy_manifests_on_group_id_status_read_at_id' + OLD_MANIFEST_INDEX = 'index_dependency_proxy_manifests_on_group_id_status_and_id' + + def up + add_concurrent_index :dependency_proxy_blobs, [:group_id, :status, :read_at, :id], name: NEW_BLOB_INDEX + add_concurrent_index :dependency_proxy_manifests, [:group_id, :status, :read_at, :id], name: NEW_MANIFEST_INDEX + + remove_concurrent_index_by_name :dependency_proxy_blobs, OLD_BLOB_INDEX + remove_concurrent_index_by_name :dependency_proxy_manifests, OLD_MANIFEST_INDEX + end + + def down + add_concurrent_index :dependency_proxy_blobs, [:group_id, :status, :id], name: OLD_BLOB_INDEX + add_concurrent_index :dependency_proxy_manifests, [:group_id, :status, :id], name: OLD_MANIFEST_INDEX + + remove_concurrent_index_by_name :dependency_proxy_blobs, NEW_BLOB_INDEX + remove_concurrent_index_by_name :dependency_proxy_manifests, NEW_MANIFEST_INDEX + end +end diff --git a/db/schema_migrations/20211105125756 b/db/schema_migrations/20211105125756 new file mode 100644 index 000000000000..842187d9ae25 --- /dev/null +++ b/db/schema_migrations/20211105125756 @@ -0,0 +1 @@ +13cf3d164d541df48b6d14d7cc1953113476ba8ea5975d7d0c5f84098e2e0e61 \ No newline at end of file diff --git a/db/schema_migrations/20211105125813 b/db/schema_migrations/20211105125813 new file mode 100644 index 000000000000..449c3d95d6ae --- /dev/null +++ b/db/schema_migrations/20211105125813 @@ -0,0 +1 @@ +a48f62bed7e4c4a0e69acd3b340065317aff71602e696970276a4e443f1dcabf \ No newline at end of file diff --git a/db/schema_migrations/20211108203248 b/db/schema_migrations/20211108203248 new file mode 100644 index 000000000000..4f8c570b627f --- /dev/null +++ b/db/schema_migrations/20211108203248 @@ -0,0 +1 @@ +a2556a3d8b21e59caa6cbf7f83d621fef391904d0c13c77c0e5da713a580b4c9 \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index 4279fbfd448f..bdcef18917c2 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -13235,7 +13235,8 @@ CREATE TABLE dependency_proxy_blobs ( file_store integer, file_name character varying NOT NULL, file text NOT NULL, - status smallint DEFAULT 0 NOT NULL + status smallint DEFAULT 0 NOT NULL, + read_at timestamp with time zone DEFAULT now() NOT NULL ); CREATE SEQUENCE dependency_proxy_blobs_id_seq @@ -13284,6 +13285,7 @@ CREATE TABLE dependency_proxy_manifests ( digest text NOT NULL, content_type text, status smallint DEFAULT 0 NOT NULL, + read_at timestamp with time zone DEFAULT now() NOT NULL, CONSTRAINT check_079b293a7b CHECK ((char_length(file) <= 255)), CONSTRAINT check_167a9a8a91 CHECK ((char_length(content_type) <= 255)), CONSTRAINT check_c579e3f586 CHECK ((char_length(file_name) <= 255)), @@ -25635,13 +25637,13 @@ CREATE UNIQUE INDEX index_dep_prox_manifests_on_group_id_file_name_and_status ON CREATE INDEX index_dependency_proxy_blobs_on_group_id_and_file_name ON dependency_proxy_blobs USING btree (group_id, file_name); -CREATE INDEX index_dependency_proxy_blobs_on_group_id_status_and_id ON dependency_proxy_blobs USING btree (group_id, status, id); +CREATE INDEX index_dependency_proxy_blobs_on_group_id_status_read_at_id ON dependency_proxy_blobs USING btree (group_id, status, read_at, id); CREATE INDEX index_dependency_proxy_blobs_on_status ON dependency_proxy_blobs USING btree (status); CREATE INDEX index_dependency_proxy_group_settings_on_group_id ON dependency_proxy_group_settings USING btree (group_id); -CREATE INDEX index_dependency_proxy_manifests_on_group_id_status_and_id ON dependency_proxy_manifests USING btree (group_id, status, id); +CREATE INDEX index_dependency_proxy_manifests_on_group_id_status_read_at_id ON dependency_proxy_manifests USING btree (group_id, status, read_at, id); CREATE INDEX index_dependency_proxy_manifests_on_status ON dependency_proxy_manifests USING btree (status); diff --git a/spec/services/dependency_proxy/find_or_create_blob_service_spec.rb b/spec/services/dependency_proxy/find_or_create_blob_service_spec.rb index 20b0546effa2..5f7afdf699a3 100644 --- a/spec/services/dependency_proxy/find_or_create_blob_service_spec.rb +++ b/spec/services/dependency_proxy/find_or_create_blob_service_spec.rb @@ -39,7 +39,7 @@ let(:blob_sha) { blob.file_name.sub('.gz', '') } it 'uses cached blob instead of downloading one' do - expect { subject }.to change { blob.reload.updated_at } + expect { subject }.to change { blob.reload.read_at } expect(subject[:status]).to eq(:success) expect(subject[:blob]).to be_a(DependencyProxy::Blob) diff --git a/spec/services/dependency_proxy/find_or_create_manifest_service_spec.rb b/spec/services/dependency_proxy/find_or_create_manifest_service_spec.rb index 407ee42d345f..ef608c9b113b 100644 --- a/spec/services/dependency_proxy/find_or_create_manifest_service_spec.rb +++ b/spec/services/dependency_proxy/find_or_create_manifest_service_spec.rb @@ -84,7 +84,7 @@ shared_examples 'using the cached manifest' do it 'uses cached manifest instead of downloading one', :aggregate_failures do - subject + expect { subject }.to change { dependency_proxy_manifest.reload.read_at } expect(subject[:status]).to eq(:success) expect(subject[:manifest]).to be_a(DependencyProxy::Manifest) diff --git a/spec/support/shared_examples/models/concerns/ttl_expirable_shared_examples.rb b/spec/support/shared_examples/models/concerns/ttl_expirable_shared_examples.rb index a4e0d6c871e6..2d08de297a35 100644 --- a/spec/support/shared_examples/models/concerns/ttl_expirable_shared_examples.rb +++ b/spec/support/shared_examples/models/concerns/ttl_expirable_shared_examples.rb @@ -11,18 +11,18 @@ it { is_expected.to validate_presence_of(:status) } end - describe '.updated_before' do + describe '.read_before' do # rubocop:disable Rails/SaveBang let_it_be_with_reload(:item1) { create(class_symbol) } let_it_be(:item2) { create(class_symbol) } # rubocop:enable Rails/SaveBang before do - item1.update_column(:updated_at, 1.month.ago) + item1.update_column(:read_at, 1.month.ago) end it 'returns items with created at older than the supplied number of days' do - expect(described_class.updated_before(10)).to contain_exactly(item1) + expect(described_class.read_before(10)).to contain_exactly(item1) end end @@ -48,4 +48,13 @@ expect(described_class.lock_next_by(:created_at)).to contain_exactly(item3) end end + + describe '#read', :freeze_time do + let_it_be(:old_read_at) { 1.day.ago } + let_it_be(:item1) { create(class_symbol, read_at: old_read_at) } + + it 'updates read_at' do + expect { item1.read! }.to change { item1.reload.read_at } + end + end end diff --git a/spec/workers/dependency_proxy/image_ttl_group_policy_worker_spec.rb b/spec/workers/dependency_proxy/image_ttl_group_policy_worker_spec.rb index d3234f4c212b..ae0cb097ebf0 100644 --- a/spec/workers/dependency_proxy/image_ttl_group_policy_worker_spec.rb +++ b/spec/workers/dependency_proxy/image_ttl_group_policy_worker_spec.rb @@ -12,8 +12,8 @@ subject { worker.perform } context 'when there are images to expire' do - let_it_be_with_reload(:old_blob) { create(:dependency_proxy_blob, group: group, updated_at: 1.year.ago) } - let_it_be_with_reload(:old_manifest) { create(:dependency_proxy_manifest, group: group, updated_at: 1.year.ago) } + let_it_be_with_reload(:old_blob) { create(:dependency_proxy_blob, group: group, read_at: 1.year.ago) } + let_it_be_with_reload(:old_manifest) { create(:dependency_proxy_manifest, group: group, read_at: 1.year.ago) } let_it_be_with_reload(:new_blob) { create(:dependency_proxy_blob, group: group) } let_it_be_with_reload(:new_manifest) { create(:dependency_proxy_manifest, group: group) }