diff --git a/app/models/pages_deployment.rb b/app/models/pages_deployment.rb index 08e541f3b93efe4b08667d6ddbc7fffaab92eab7..fb13dbfb8cac74df53f1d4d8862d73661d8df4ef 100644 --- a/app/models/pages_deployment.rb +++ b/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 } diff --git a/app/models/project.rb b/app/models/project.rb index c43f78f3db9a38347cfb945e84ddc16810d9ac14..7e4ec6c7036664ce6e1e2e643af2daf0bb16ea2f 100644 --- a/app/models/project.rb +++ b/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 @@ def remove_pages 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 @@ def mark_pages_as_deployed(artifacts_archive: nil) 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) diff --git a/app/services/pages/destroy_deployments_service.rb b/app/services/pages/destroy_deployments_service.rb new file mode 100644 index 0000000000000000000000000000000000000000..45d906bec7ac100965ef60f1b99488e1be030ab3 --- /dev/null +++ b/app/services/pages/destroy_deployments_service.rb @@ -0,0 +1,16 @@ +# 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 diff --git a/app/services/projects/update_pages_service.rb b/app/services/projects/update_pages_service.rb index d64ad693c6c646f9906cf40e1892f0a429763e98..9a1e6595a76f6032a09abe78f2427d9aef973ca6 100644 --- a/app/services/projects/update_pages_service.rb +++ b/app/services/projects/update_pages_service.rb @@ -12,6 +12,11 @@ class UpdatePagesService < BaseService # 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 @@ def create_pages_deployment(artifacts_path, build) 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 @@ def create_pages_deployment(artifacts_path, build) 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 diff --git a/app/workers/all_queues.yml b/app/workers/all_queues.yml index 7f5fca18c4c4939213226bba2a2be76351e36902..f7ba8eb09a07cd1f3b6c46d1f715194f909dba7a 100644 --- a/app/workers/all_queues.yml +++ b/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: diff --git a/app/workers/destroy_pages_deployments_worker.rb b/app/workers/destroy_pages_deployments_worker.rb new file mode 100644 index 0000000000000000000000000000000000000000..32b539325c9020727d89b5726d0d96a6769df255 --- /dev/null +++ b/app/workers/destroy_pages_deployments_worker.rb @@ -0,0 +1,19 @@ +# 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 diff --git a/config/sidekiq_queues.yml b/config/sidekiq_queues.yml index 258a75f47fb62e815e396a3f2f87c7bd913559d9..ded8b769ff192d8d71aad161a6d44080b630d047 100644 --- a/config/sidekiq_queues.yml +++ b/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 diff --git a/spec/models/pages_deployment_spec.rb b/spec/models/pages_deployment_spec.rb index 057c01d26cf513fddf92f0bb922c2083c2e503ab..e059b477e0eaff481f19c236d0713d5ca1d2b69c 100644 --- a/spec/models/pages_deployment_spec.rb +++ b/spec/models/pages_deployment_spec.rb @@ -42,4 +42,17 @@ 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 diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 5f643fb03dfeb5e6d957fadb080f55c4faadd905..5e2711c791ef3c5792c36c30b80be6823dff6856 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -3668,7 +3668,7 @@ 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 @@ def enable_lfs 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 diff --git a/spec/services/pages/destroy_deployments_service_spec.rb b/spec/services/pages/destroy_deployments_service_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..0f8e8b6573eeee9e369113ea667f82e730baca6b --- /dev/null +++ b/spec/services/pages/destroy_deployments_service_spec.rb @@ -0,0 +1,29 @@ +# 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 diff --git a/spec/services/projects/update_pages_service_spec.rb b/spec/services/projects/update_pages_service_spec.rb index d8be4d47437d05c853064e30e44b1c1f8f0bbe79..92772136d697217d9a403289aa7f201d6d3ea380 100644 --- a/spec/services/projects/update_pages_service_spec.rb +++ b/spec/services/projects/update_pages_service_spec.rb @@ -71,6 +71,29 @@ 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) diff --git a/spec/workers/destroy_pages_deployments_worker_spec.rb b/spec/workers/destroy_pages_deployments_worker_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..2c20c9004efa4a5e34817fc2638e837b652fc273 --- /dev/null +++ b/spec/workers/destroy_pages_deployments_worker_spec.rb @@ -0,0 +1,38 @@ +# 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