Merge branch '300615-stop-removing-pages-from-disk-if-feature-flag-is-disabled' into 'master'

Mark pages as not deployed and remove domains synchrounsly

See merge request gitlab-org/gitlab!53137

app/models/project.rb

@@ -117,7 +117,7 @@ class Project < ApplicationRecord
 
   use_fast_destroy :build_trace_chunks
 
-  after_destroy -> { run_after_commit { remove_pages } }
+  after_destroy -> { run_after_commit { legacy_remove_pages } }
   after_destroy :remove_exports
 
   after_validation :check_pending_delete
@@ -1788,16 +1788,16 @@ class Project < ApplicationRecord
                .delete_all
   end
 
-  # TODO: what to do here when not using Legacy Storage? Do we still need to rename and delay removal?
+  # TODO: remove this method https://gitlab.com/gitlab-org/gitlab/-/issues/320775
   # rubocop: disable CodeReuse/ServiceClass
-  def remove_pages
+  def legacy_remove_pages
+    return unless Feature.enabled?(:pages_update_legacy_storage, default_enabled: true)
+
     # Projects with a missing namespace cannot have their pages removed
     return unless namespace
 
     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

app/services/pages/delete_service.rb

@@ -3,7 +3,13 @@
 module Pages
   class DeleteService < BaseService
     def execute
-      PagesRemoveWorker.perform_async(project.id)
+      project.mark_pages_as_not_deployed # prevents domain from updating config when deleted
+      project.pages_domains.delete_all
+
+      DestroyPagesDeploymentsWorker.perform_async(project.id)
+
+      # TODO: remove this call https://gitlab.com/gitlab-org/gitlab/-/issues/320775
+      PagesRemoveWorker.perform_async(project.id) if Feature.enabled?(:pages_update_legacy_storage, default_enabled: true)
     end
   end
 end

app/workers/pages_remove_worker.rb

 # frozen_string_literal: true
 
+# TODO: remove this worker https://gitlab.com/gitlab-org/gitlab/-/issues/320775
 class PagesRemoveWorker # rubocop:disable Scalability/IdempotentWorker
   include ApplicationWorker
 
@@ -11,7 +12,6 @@ class PagesRemoveWorker # rubocop:disable Scalability/IdempotentWorker
     project = Project.find_by_id(project_id)
     return unless project
 
-    project.remove_pages
-    project.pages_domains.delete_all
+    project.legacy_remove_pages
   end
 end

spec/models/project_spec.rb

@@ -4117,7 +4117,7 @@ RSpec.describe Project, factory_default: :keep do
     end
   end
 
-  describe '#remove_pages' do
+  describe '#legacy_remove_pages' do
     let(:project) { create(:project).tap { |project| project.mark_pages_as_deployed } }
     let(:pages_metadatum) { project.pages_metadatum }
     let(:namespace) { project.namespace }
@@ -4136,34 +4136,22 @@ RSpec.describe Project, factory_default: :keep do
       expect_any_instance_of(Gitlab::PagesTransfer).to receive(:rename_project).and_return(true)
       expect(PagesWorker).to receive(:perform_in).with(5.minutes, :remove, namespace.full_path, anything)
 
-      expect { project.remove_pages }.to change { pages_metadatum.reload.deployed }.from(true).to(false)
+      expect { project.legacy_remove_pages }.to change { pages_metadatum.reload.deployed }.from(true).to(false)
     end
 
-    it 'is run when the project is destroyed' do
-      expect(project).to receive(:remove_pages).and_call_original
-
-      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 'does nothing if updates on legacy storage are disabled' do
+      stub_feature_flags(pages_update_legacy_storage: false)
 
-      it 'schedules a destruction of pages deployments' do
-        expect(DestroyPagesDeploymentsWorker).to(
-          receive(:perform_async).with(project.id)
-        )
+      expect(Gitlab::PagesTransfer).not_to receive(:new)
+      expect(PagesWorker).not_to receive(:perform_in)
 
-        project.remove_pages
-      end
+      project.legacy_remove_pages
+    end
 
-      it 'removes pages deployments', :sidekiq_inline do
-        expect do
-          project.remove_pages
-        end.to change { PagesDeployment.count }.by(-1)
+    it 'is run when the project is destroyed' do
+      expect(project).to receive(:legacy_remove_pages).and_call_original
 
-        expect(PagesDeployment.find_by_id(old_deployment.id)).to be_nil
-      end
+      expect { project.destroy }.not_to raise_error
     end
   end
 

spec/services/pages/delete_services_spec.rb

@@ -3,35 +3,74 @@
 require 'spec_helper'
 
 RSpec.describe Pages::DeleteService do
-  shared_examples 'remove pages' do
-    let_it_be(:project) { create(:project, path: "my.project")}
-    let_it_be(:admin) { create(:admin) }
-    let_it_be(:domain) { create(:pages_domain, project: project) }
-    let_it_be(:service) { described_class.new(project, admin)}
+  let_it_be(:admin) { create(:admin) }
 
-    it 'deletes published pages' do
-      expect_any_instance_of(Gitlab::PagesTransfer).to receive(:rename_project).and_return true
-      expect(PagesWorker).to receive(:perform_in).with(5.minutes, :remove, project.namespace.full_path, anything)
+  let(:project) { create(:project, path: "my.project")}
+  let!(:domain) { create(:pages_domain, project: project) }
+  let(:service) { described_class.new(project, admin)}
 
-      Sidekiq::Testing.inline! { service.execute }
+  before do
+    project.mark_pages_as_deployed
+  end
+
+  it 'deletes published pages', :sidekiq_inline do
+    expect(project.pages_deployed?).to be(true)
+
+    expect_any_instance_of(Gitlab::PagesTransfer).to receive(:rename_project).and_return true
+    expect(PagesWorker).to receive(:perform_in).with(5.minutes, :remove, project.namespace.full_path, anything)
+
+    service.execute
+
+    expect(project.pages_deployed?).to be(false)
+  end
+
+  it "doesn't remove anything from the legacy storage if updates on it are disabled", :sidekiq_inline do
+    stub_feature_flags(pages_update_legacy_storage: false)
+
+    expect(project.pages_deployed?).to be(true)
+
+    expect(PagesWorker).not_to receive(:perform_in)
+
+    service.execute
 
-      expect(project.reload.pages_metadatum.deployed?).to be(false)
-    end
+    expect(project.pages_deployed?).to be(false)
+  end
+
+  it 'deletes all domains', :sidekiq_inline do
+    expect(project.pages_domains.count).to eq(1)
+
+    service.execute
+
+    expect(project.reload.pages_domains.count).to eq(0)
+  end
 
-    it 'deletes all domains' do
-      expect(project.pages_domains.count).to be 1
+  it 'schedules a destruction of pages deployments' do
+    expect(DestroyPagesDeploymentsWorker).to(
+      receive(:perform_async).with(project.id)
+    )
 
-      Sidekiq::Testing.inline! { service.execute }
+    service.execute
+  end
+
+  it 'removes pages deployments', :sidekiq_inline do
+    create(:pages_deployment, project: project)
 
-      expect(project.reload.pages_domains.count).to be 0
-    end
+    expect do
+      service.execute
+    end.to change { PagesDeployment.count }.by(-1)
   end
 
-  context 'with feature flag enabled' do
-    before do
-      expect(PagesRemoveWorker).to receive(:perform_async).and_call_original
-    end
+  it 'marks pages as not deployed, deletes domains and schedules worker to remove pages from disk' do
+    expect(project.pages_deployed?).to eq(true)
+    expect(project.pages_domains.count).to eq(1)
+
+    service.execute
+
+    expect(project.pages_deployed?).to eq(false)
+    expect(project.pages_domains.count).to eq(0)
+
+    expect_any_instance_of(Gitlab::PagesTransfer).to receive(:rename_project).and_return true
 
-    it_behaves_like 'remove pages'
+    Sidekiq::Worker.drain_all
   end
 end

spec/services/projects/update_pages_service_spec.rb

@@ -16,7 +16,7 @@ RSpec.describe Projects::UpdatePagesService do
   subject { described_class.new(project, build) }
 
   before do
-    project.remove_pages
+    project.legacy_remove_pages
   end
 
   context '::TMP_EXTRACT_PATH' do

spec/workers/namespaceless_project_destroy_worker_spec.rb

@@ -49,7 +49,7 @@ RSpec.describe NamespacelessProjectDestroyWorker do
           subject.perform(project.id)
         end
 
-        it 'does not do anything in Project#remove_pages method' do
+        it 'does not do anything in Project#legacy_remove_pages method' do
           expect(Gitlab::PagesTransfer).not_to receive(:new)
 
           subject.perform(project.id)

spec/workers/pages_remove_worker_spec.rb

@@ -3,24 +3,23 @@
 require 'spec_helper'
 
 RSpec.describe PagesRemoveWorker do
-  let_it_be(:project) { create(:project, path: "my.project")}
-  let_it_be(:domain) { create(:pages_domain, project: project) }
+  let(:project) { create(:project, path: "my.project")}
+  let!(:domain) { create(:pages_domain, project: project) }
+
   subject { described_class.new.perform(project.id) }
 
+  before do
+    project.mark_pages_as_deployed
+  end
+
   it 'deletes published pages' do
+    expect(project.pages_deployed?).to be(true)
+
     expect_any_instance_of(Gitlab::PagesTransfer).to receive(:rename_project).and_return true
     expect(PagesWorker).to receive(:perform_in).with(5.minutes, :remove, project.namespace.full_path, anything)
 
     subject
 
-    expect(project.reload.pages_metadatum.deployed?).to be(false)
-  end
-
-  it 'deletes all domains' do
-    expect(project.pages_domains.count).to be 1
-
-    subject
-
-    expect(project.reload.pages_domains.count).to be 0
+    expect(project.reload.pages_deployed?).to be(false)
   end
 end