Merge branch '208193-add-expiration-started-at-to-container-repositories' into 'master'

Add expiration_policy_started_at support in container repositories

See merge request gitlab-org/gitlab!42598

app/models/container_repository.rb

@@ -107,6 +107,14 @@ class ContainerRepository < ApplicationRecord
     client.delete_repository_tag_by_name(self.path, name)
   end
 
+  def reset_expiration_policy_started_at!
+    update!(expiration_policy_started_at: nil)
+  end
+
+  def start_expiration_policy!
+    update!(expiration_policy_started_at: Time.zone.now)
+  end
+
   def self.build_from_path(path)
     self.new(project: path.repository_project,
              name: path.repository_name)

app/workers/cleanup_container_repository_worker.rb

@@ -16,9 +16,17 @@ class CleanupContainerRepositoryWorker # rubocop:disable Scalability/IdempotentW
 
     return unless valid?
 
-    Projects::ContainerRepository::CleanupTagsService
+    if run_by_container_expiration_policy?
+      container_repository.start_expiration_policy!
+    end
+
+    result = Projects::ContainerRepository::CleanupTagsService
       .new(project, current_user, params)
       .execute(container_repository)
+
+    if run_by_container_expiration_policy? && result[:status] == :success
+      container_repository.reset_expiration_policy_started_at!
+    end
   end
 
   private
@@ -30,7 +38,7 @@ class CleanupContainerRepositoryWorker # rubocop:disable Scalability/IdempotentW
   end
 
   def run_by_container_expiration_policy?
-    @params['container_expiration_policy'] && container_repository && project
+    @params['container_expiration_policy'] && container_repository.present? && project.present?
   end
 
   def project

changelogs/unreleased/208193-add-expiration-started-at-to-container-repositories.yml

+---
+title: Add expiration policy started at support in container repositories
+merge_request: 42598
+author:
+type: added

db/migrate/20200914070140_add_expiration_policy_started_at_to_container_repositories.rb

+# frozen_string_literal: true
+
+class AddExpirationPolicyStartedAtToContainerRepositories < ActiveRecord::Migration[6.0]
+  DOWNTIME = false
+
+  def up
+    add_column(:container_repositories, :expiration_policy_started_at, :datetime_with_timezone)
+  end
+
+  def down
+    remove_column(:container_repositories, :expiration_policy_started_at)
+  end
+end

db/schema_migrations/20200914070140

+8cabe224e89ef77fe4a2bc1f8bf7381faaa10d1cab4907a26aeb2162a126af52
\ No newline at end of file

db/structure.sql

@@ -11083,7 +11083,8 @@ CREATE TABLE container_repositories (
     name character varying NOT NULL,
     created_at timestamp without time zone NOT NULL,
     updated_at timestamp without time zone NOT NULL,
-    status smallint
+    status smallint,
+    expiration_policy_started_at timestamp with time zone
 );
 
 CREATE SEQUENCE container_repositories_id_seq

doc/api/container_registry.md

@@ -41,7 +41,8 @@ Example response:
     "path": "group/project",
     "project_id": 9,
     "location": "gitlab.example.com:5000/group/project",
-    "created_at": "2019-01-10T13:38:57.391Z"
+    "created_at": "2019-01-10T13:38:57.391Z",
+    "cleanup_policy_started_at": "2020-01-10T15:40:57.391Z"
   },
   {
     "id": 2,
@@ -49,7 +50,8 @@ Example response:
     "path": "group/project/releases",
     "project_id": 9,
     "location": "gitlab.example.com:5000/group/project/releases",
-    "created_at": "2019-01-10T13:39:08.229Z"
+    "created_at": "2019-01-10T13:39:08.229Z",
+    "cleanup_policy_started_at": "2020-08-17T03:12:35.489Z"
   }
 ]
 ```
@@ -84,6 +86,7 @@ Example response:
     "project_id": 9,
     "location": "gitlab.example.com:5000/group/project",
     "created_at": "2019-01-10T13:38:57.391Z",
+    "cleanup_policy_started_at": "2020-08-17T03:12:35.489Z",
     "tags_count": 1,
     "tags": [
       {
@@ -100,6 +103,7 @@ Example response:
     "project_id": 11,
     "location": "gitlab.example.com:5000/group/other_project",
     "created_at": "2019-01-10T13:39:08.229Z",
+    "cleanup_policy_started_at": "2020-01-10T15:40:57.391Z",
     "tags_count": 3,
     "tags": [
       {

lib/api/entities/container_registry.rb

@@ -16,6 +16,7 @@ module API
         expose :project_id
         expose :location
         expose :created_at
+        expose :expiration_policy_started_at, as: :cleanup_policy_started_at
         expose :tags_count, if: -> (_, options) { options[:tags_count] }
         expose :tags, using: Tag, if: -> (_, options) { options[:tags] }
       end

spec/fixtures/api/schemas/registry/repository.json

@@ -20,6 +20,9 @@
     "created_at": {
       "type": "date-time"
     },
+    "cleanup_policy_started_at": {
+      "type": "date-time"
+    },
     "tags_path": {
       "type": "string"
     },

spec/models/container_repository_spec.rb

@@ -184,6 +184,33 @@ RSpec.describe ContainerRepository do
     end
   end
 
+  describe '#start_expiration_policy!' do
+    subject { repository.start_expiration_policy! }
+
+    it 'sets the expiration policy started at to now' do
+      Timecop.freeze do
+        expect { subject }
+          .to change { repository.expiration_policy_started_at }.from(nil).to(Time.zone.now)
+      end
+    end
+  end
+
+  describe '#reset_expiration_policy_started_at!' do
+    subject { repository.reset_expiration_policy_started_at! }
+
+    before do
+      repository.start_expiration_policy!
+    end
+
+    it 'resets the expiration policy started at' do
+      started_at = repository.expiration_policy_started_at
+
+      expect(started_at).not_to be_nil
+      expect { subject }
+          .to change { repository.expiration_policy_started_at }.from(started_at).to(nil)
+    end
+  end
+
   describe '.build_from_path' do
     let(:registry_path) do
       ContainerRegistry::Path.new(project.full_path + '/some/image')

spec/workers/cleanup_container_repository_worker_spec.rb

@@ -40,14 +40,35 @@ RSpec.describe CleanupContainerRepositoryWorker, :clean_gitlab_redis_shared_stat
     context 'container expiration policy' do
       let(:params) { { key: 'value', 'container_expiration_policy' => true } }
 
+      before do
+        allow(ContainerRepository)
+          .to receive(:find_by_id).with(repository.id).and_return(repository)
+      end
+
       it 'executes the destroy service' do
+        expect(repository).to receive(:start_expiration_policy!).and_call_original
+        expect(repository).to receive(:reset_expiration_policy_started_at!).and_call_original
         expect(Projects::ContainerRepository::CleanupTagsService).to receive(:new)
           .with(project, nil, params.merge('container_expiration_policy' => true))
           .and_return(service)
 
-        expect(service).to receive(:execute)
+        expect(service).to receive(:execute).and_return(status: :success)
+
+        subject.perform(nil, repository.id, params)
+        expect(repository.reload.expiration_policy_started_at).to be_nil
+      end
+
+      it "doesn't reset the expiration policy started at if the destroy service returns an error" do
+        expect(repository).to receive(:start_expiration_policy!).and_call_original
+        expect(repository).not_to receive(:reset_expiration_policy_started_at!)
+        expect(Projects::ContainerRepository::CleanupTagsService).to receive(:new)
+          .with(project, nil, params.merge('container_expiration_policy' => true))
+          .and_return(service)
+
+        expect(service).to receive(:execute).and_return(status: :error, message: 'timeout while deleting tags')
 
         subject.perform(nil, repository.id, params)
+        expect(repository.reload.expiration_policy_started_at).not_to be_nil
       end
     end
   end