Merge branch '341536-dp-read-at-for-ttl' into 'master'

Add read_at to dependency proxy objects

See merge request gitlab-org/gitlab!73842

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

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

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.read! if from_cache
-      blob.touch if from_cache
       success(blob: blob, from_cache: from_cache)
     end
 

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)

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.read_before(policy.ttl)
-        qualified_blobs = policy.group.dependency_proxy_blobs.active.updated_before(policy.ttl)
+        qualified_manifests = policy.group.dependency_proxy_manifests.active.read_before(policy.ttl)
-        qualified_manifests = policy.group.dependency_proxy_manifests.active.updated_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)

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

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

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

db/schema_migrations/20211105125756

@@ -0,0 +1 @@
+13cf3d164d541df48b6d14d7cc1953113476ba8ea5975d7d0c5f84098e2e0e61

db/schema_migrations/20211105125813

@@ -0,0 +1 @@
+a48f62bed7e4c4a0e69acd3b340065317aff71602e696970276a4e443f1dcabf

db/schema_migrations/20211108203248

@@ -0,0 +1 @@
+a2556a3d8b21e59caa6cbf7f83d621fef391904d0c13c77c0e5da713a580b4c9

db/structure.sql

@@ -13237,7 +13237,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
@@ -13286,6 +13287,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)),
@@ -25646,13 +25648,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);
 

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)

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)

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

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_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, updated_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) }