Merge branch '235730-clean-old-pages-deployments-on-new-deployment' into 'master'

Clean old pages deployments on new deployment

See merge request gitlab-org/gitlab!44575

app/models/pages_deployment.rb

@@ -7,6 +7,8 @@ class PagesDeployment < ApplicationRecord
   belongs_to :project, optional: false
   belongs_to :ci_build, class_name: 'Ci::Build', optional: true
 
+  scope :older_than, -> (id) { where('id < ?', id) }
+
   validates :file, presence: true
   validates :file_store, presence: true, inclusion: { in: ObjectStorage::SUPPORTED_STORES }
   validates :size, presence: true, numericality: { greater_than: 0, only_integer: true }

app/models/project.rb

@@ -346,7 +346,8 @@ class Project < ApplicationRecord
   # GitLab Pages
   has_many :pages_domains
   has_one  :pages_metadatum, class_name: 'ProjectPagesMetadatum', inverse_of: :project
-  has_many :pages_deployments
+  # we need to clean up files, not only remove records
+  has_many :pages_deployments, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
 
   # Can be too many records. We need to implement delete_all in batches.
   # Issue https://gitlab.com/gitlab-org/gitlab/-/issues/228637
@@ -1801,6 +1802,8 @@ class Project < ApplicationRecord
 
     mark_pages_as_not_deployed unless destroyed?
 
+    DestroyPagesDeploymentsWorker.perform_async(id)
+
     # 1. We rename pages to temporary directory
     # 2. We wait 5 minutes, due to NFS caching
     # 3. We asynchronously remove pages with force
@@ -1817,7 +1820,7 @@ class Project < ApplicationRecord
   end
 
   def mark_pages_as_not_deployed
-    ensure_pages_metadatum.update!(deployed: false, artifacts_archive: nil)
+    ensure_pages_metadatum.update!(deployed: false, artifacts_archive: nil, pages_deployment: nil)
   end
 
   def write_repository_config(gl_full_path: full_path)

app/services/pages/destroy_deployments_service.rb

+# frozen_string_literal: true
+
+module Pages
+  class DestroyDeploymentsService
+    def initialize(project, last_deployment_id = nil)
+      @project = project
+      @last_deployment_id = last_deployment_id
+    end
+
+    def execute
+      deployments_to_destroy = @project.pages_deployments
+      deployments_to_destroy = deployments_to_destroy.older_than(@last_deployment_id) if @last_deployment_id
+      deployments_to_destroy.find_each(&:destroy) # rubocop: disable CodeReuse/ActiveRecord
+    end
+  end
+end

app/services/projects/update_pages_service.rb

@@ -12,6 +12,11 @@ module Projects
     # as it shares the namespace with groups
     TMP_EXTRACT_PATH = '@pages.tmp'
 
+    # old deployment can be cached by pages daemon
+    # so we need to give pages daemon some time update cache
+    # 10 minutes is enough, but 30 feels safer
+    OLD_DEPLOYMENTS_DESTRUCTION_DELAY = 30.minutes.freeze
+
     attr_reader :build
 
     def initialize(project, build)
@@ -128,6 +133,7 @@ module Projects
       entries_count = build.artifacts_metadata_entry("", recursive: true).entries.count
       sha256 = build.job_artifacts_archive.file_sha256
 
+      deployment = nil
       File.open(artifacts_path) do |file|
         deployment = project.pages_deployments.create!(file: file,
                                                        file_count: entries_count,
@@ -135,7 +141,11 @@ module Projects
         project.pages_metadatum.update!(pages_deployment: deployment)
       end
 
-      # TODO: schedule old deployment removal https://gitlab.com/gitlab-org/gitlab/-/issues/235730
+      DestroyPagesDeploymentsWorker.perform_in(
+        OLD_DEPLOYMENTS_DESTRUCTION_DELAY,
+        project.id,
+        deployment.id
+      )
     rescue => e
       # we don't want to break current pages deployment process if something goes wrong
       # TODO: remove this rescue as part of https://gitlab.com/gitlab-org/gitlab/-/issues/245308

app/workers/all_queues.yml

@@ -1425,6 +1425,14 @@
   :weight: 1
   :idempotent:
   :tags: []
+- :name: destroy_pages_deployments
+  :feature_category: :pages
+  :has_external_dependencies:
+  :urgency: :low
+  :resource_boundary: :unknown
+  :weight: 1
+  :idempotent: true
+  :tags: []
 - :name: detect_repository_languages
   :feature_category: :source_code_management
   :has_external_dependencies:

app/workers/destroy_pages_deployments_worker.rb

+# frozen_string_literal: true
+
+class DestroyPagesDeploymentsWorker
+  include ApplicationWorker
+
+  idempotent!
+
+  loggable_arguments 0, 1
+  sidekiq_options retry: 3
+  feature_category :pages
+
+  def perform(project_id, last_deployment_id = nil)
+    project = Project.find_by_id(project_id)
+
+    return unless project
+
+    ::Pages::DestroyDeploymentsService.new(project, last_deployment_id).execute
+  end
+end

config/sidekiq_queues.yml

@@ -86,6 +86,8 @@
   - 1
 - - design_management_new_version
   - 1
+- - destroy_pages_deployments
+  - 1
 - - detect_repository_languages
   - 1
 - - disallow_two_factor_for_group

spec/models/pages_deployment_spec.rb

@@ -42,4 +42,17 @@ RSpec.describe PagesDeployment do
     deployment = create(:pages_deployment)
     expect(deployment.size).to eq(deployment.file.size)
   end
+
+  describe '.older_than' do
+    it 'returns deployments with lower id' do
+      old_deployments = create_list(:pages_deployment, 2)
+
+      deployment = create(:pages_deployment)
+
+      # new deployment
+      create(:pages_deployment)
+
+      expect(PagesDeployment.older_than(deployment.id)).to eq(old_deployments)
+    end
+  end
 end

spec/models/project_spec.rb

@@ -3668,7 +3668,7 @@ RSpec.describe Project, factory_default: :keep do
     let(:project) { create(:project) }
 
     before do
-      project.namespace_id = 7
+      project.namespace_id = project.namespace_id + 1
     end
 
     it { expect(project.parent_changed?).to be_truthy }
@@ -4219,6 +4219,27 @@ RSpec.describe Project, factory_default: :keep do
 
       expect { project.destroy }.not_to raise_error
     end
+
+    context 'when there is an old pages deployment' do
+      let!(:old_deployment_from_another_project) { create(:pages_deployment) }
+      let!(:old_deployment) { create(:pages_deployment, project: project) }
+
+      it 'schedules a destruction of pages deployments' do
+        expect(DestroyPagesDeploymentsWorker).to(
+          receive(:perform_async).with(project.id)
+        )
+
+        project.remove_pages
+      end
+
+      it 'removes pages deployments', :sidekiq_inline do
+        expect do
+          project.remove_pages
+        end.to change { PagesDeployment.count }.by(-1)
+
+        expect(PagesDeployment.find_by_id(old_deployment.id)).to be_nil
+      end
+    end
   end
 
   describe '#remove_export' do

spec/services/pages/destroy_deployments_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Pages::DestroyDeploymentsService do
+  let(:project) { create(:project) }
+  let!(:old_deployments) { create_list(:pages_deployment, 2, project: project) }
+  let!(:last_deployment) { create(:pages_deployment, project: project) }
+  let!(:newer_deployment) { create(:pages_deployment, project: project) }
+  let!(:deployment_from_another_project) { create(:pages_deployment) }
+
+  it 'destroys all deployments of the project' do
+    expect do
+      described_class.new(project).execute
+    end.to change { PagesDeployment.count }.by(-4)
+
+    expect(deployment_from_another_project.reload).to be
+  end
+
+  it 'destroy only deployments older than last deployment if it is provided' do
+    expect do
+      described_class.new(project, last_deployment.id).execute
+    end.to change { PagesDeployment.count }.by(-2)
+
+    expect(last_deployment.reload).to be
+    expect(newer_deployment.reload).to be
+    expect(deployment_from_another_project.reload).to be
+  end
+end

spec/services/projects/update_pages_service_spec.rb

@@ -71,6 +71,29 @@ RSpec.describe Projects::UpdatePagesService do
         expect(project.pages_metadatum.reload.pages_deployment_id).to eq(deployment.id)
       end
 
+      context 'when there is an old pages deployment' do
+        let!(:old_deployment_from_another_project) { create(:pages_deployment) }
+        let!(:old_deployment) { create(:pages_deployment, project: project) }
+
+        it 'schedules a destruction of older deployments' do
+          expect(DestroyPagesDeploymentsWorker).to(
+            receive(:perform_in).with(described_class::OLD_DEPLOYMENTS_DESTRUCTION_DELAY,
+                                      project.id,
+                                      instance_of(Integer))
+          )
+
+          execute
+        end
+
+        it 'removes older deployments', :sidekiq_inline do
+          expect do
+            execute
+          end.not_to change { PagesDeployment.count } # it creates one and deletes one
+
+          expect(PagesDeployment.find_by_id(old_deployment.id)).to be_nil
+        end
+      end
+
       it 'does not create deployment when zip_pages_deployments feature flag is disabled' do
         stub_feature_flags(zip_pages_deployments: false)
 

spec/workers/destroy_pages_deployments_worker_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe DestroyPagesDeploymentsWorker do
+  subject(:worker) { described_class.new }
+
+  let(:project) { create(:project) }
+  let!(:old_deployment) { create(:pages_deployment, project: project) }
+  let!(:last_deployment) { create(:pages_deployment, project: project) }
+  let!(:another_deployment) { create(:pages_deployment) }
+
+  it "doesn't fail if project is already removed" do
+    expect do
+      worker.perform(-1)
+    end.not_to raise_error
+  end
+
+  it 'can be called without last_deployment_id' do
+    expect_next_instance_of(::Pages::DestroyDeploymentsService, project, nil) do |service|
+      expect(service).to receive(:execute).and_call_original
+    end
+
+    expect do
+      worker.perform(project.id)
+    end.to change { PagesDeployment.count }.by(-2)
+  end
+
+  it 'calls destroy service' do
+    expect_next_instance_of(::Pages::DestroyDeploymentsService, project, last_deployment.id) do |service|
+      expect(service).to receive(:execute).and_call_original
+    end
+
+    expect do
+      worker.perform(project.id, last_deployment.id)
+    end.to change { PagesDeployment.count }.by(-1)
+  end
+end